-1

There are required pieces to formulate my problem. Below are content of MyError.h header file.

myError.h

###########################
# myError.h
###########################

1  typedef enum
2  {
3     MySuccess = 0x00000000,
4     MyError1  = 0x00000001,
5     MyError2  = 0x00000003,
6     MyForce32 = 0x7FFFFFFF
7  } MyError;

8  #define PROPAGATE_ERROR_FAIL_MY_1(_err) \
9  do { \
10    e = (_err); \
11    if (e != MySuccess) \
12   { \
13       MY_UTILS_LOG_ERROR(e, __FILE__, __FUNCTION__, __LINE__, true, 0); \
14        goto fail; \
15    } \
16 } while (0)


17  #define MY_UTILS_LOG_ERROR(_err, _file, _func, _line, _propagating, _format, ...) \
18  do { \
19    MyUtilsLogError(MY_UTILS_ERROR_TAG, MY_UTILS_ERROR_PATH, \
20                            (_err), (_file), (_func), (_line), \
21                            (_propagating), (_format), ##__VA_ARGS__); \
22 } \
23 while (0)

24 void MyUtilsLogError(const char* tag, const char* path, MyError e, const char* file, const char* func,
                    uint32_t line, bool propagating, const char* format, ...)
//Here MyError is passed just to print the String for Error for example if we pass MyError1 then string MyError1 will be printed in logs on console.

Below are required pieces from MyError.c file, which simply include above header file and calls the PROPAGATE_ERROR_FAIL_MY_1 macro in APIs.

myError.c

#include "myerror.h"

static MyError foo(uint32_t x, uint32_t y) {
    if (x==y) {
        return MySuccess;
    } else {
        return MyError1;
    }
}

static MyError fooCaller(void) {
    MyError e = MySuccess;
    uint32_t x = 1U, y = 1U;

    PROPAGATE_ERROR_FAIL_MY_1(foo(x,y)); //This is where I get all kind of weird MISRA violation [1][2].

fail:
    return e;
}

NOTE: FYI MyUtilsLogError() is just a API which helps in dumping the logs on console.

In myError.c file I see below MISRA 2012 violations:

[1]: misra_c_2012_rule_10_4_violation: Essential type of the left hand operand "e" (enum) is not the same as that of the right operand "MySuccess"(boolean).

[2]: misra_c_2012_rule_11_9_violation: Literal "0" shall not be used as null pointer constant.

I'm not getting why MISRA is reporting 10.4 violation even though I'm comparing the same enum type at line#11 in myErro.h file ?

Any clue why 10.9 violation is being reported here ? Is macro not good to use for MISRA ?

Amit Sharma
  • 1,987
  • 2
  • 18
  • 29
  • 2
    1. `MYSuccess` is not the same as `MySuccess`. Capitalization matters in C. 2. 0 is passed to `MyUtilsLogError` as the parameter `_format`. What is the type of `_format`. If it is a pointer the MISRA error is saying you cannot convert `0` to a pointer – Rishikesh Raje May 22 '19 at 09:20
  • I'm no expert at MISRA, but isn't `goto` and functions with variable number of arguments also prohibited? I don't see how this would pass even if you'd fix violation errors. – user694733 May 22 '19 at 09:21
  • @RishikeshRaje Sory that is type.. let me edit the post. – Amit Sharma May 22 '19 at 09:33
  • I guess yout want to write: uint32_t y and not :uint32_y – Mike May 22 '19 at 09:39
  • I don't know if MISRA allows `goto` or `return` inside a macro, but macros that affect control flow are a **very** bad idea. It looks like a function call but exits (or jumps in) the "calling" function; don't break the internal parsers of those who will read the code. – alx - recommends codidact May 22 '19 at 10:58
  • `__FUNCTION__` is not standard. If you're using C99 or later, you should use `__func__` – alx - recommends codidact May 22 '19 at 11:05
  • This is very fishy code. Macros like these are fine in debug build, but not in your MISRA-compliant production code. It will be impossible to justify them - you should write a proper error handler instead. Instead of "goto fail" BASIC programming, you could wrap your functions in an error handler wrapper. Such as `execute(foo, params);` where foo returns an error code upon error and `execute` contains the error handler. – Lundin May 22 '19 at 11:37
  • As for your question, I would guess that in the real code you have `static bool foo (...`. Also `} while (0) fail: return e;` is not valid C and will not compile. Post the actual code giving the errors. – Lundin May 22 '19 at 11:42
  • Use `-std=c99 -pedantic -Wall -Wextra -Werror` (or the standard you use) to compile and see what happens :) – alx - recommends codidact May 22 '19 at 11:53
  • As an aside, you can drop the "do { ... } while(0)" guff... MISRA C:2012 rightly dropped that requirement :-) – Andrew Jun 12 '19 at 08:41

2 Answers2

0

in

static MyError foo(uint32_t x, uint32_y) {

you want

static MyError foo(uint32_t x, uint32_t y) {

In

     if (e != MYSuccess) \

you want

     if (e != MySuccess) \

In

 MY_UTILS_LOG_ERROR(e, __FILE__, __FUNCTION__, __LINE__, true, 0); \

there is not enough args, ISO C99 requires at least one argument for the "..." in a variadic macro

warning ISO C does not support __FUNCTION__ predefined identifier

Also MyUtilsLogError is not declared/defined of your question, what is its signature ? Does it knows at least what MyError is ?

bruno
  • 32,421
  • 7
  • 25
  • 37
  • I have fixed the typos..Thanks for pointing the typos.. We are already passing one args `PROPAGATE_ERROR_FAIL_MY_1(foo(x,y))`. `foo(x,y)` will return MyError type code which is arg here to `PROPAGATE_ERROR_FAIL_MY_1`. Am I missing something else here ? – Amit Sharma May 22 '19 at 09:52
  • @AmitSharma "_missing something else here_" you mean about the missing arg ? yes one is missing in `MY_UTILS_LOG_ERROR(e, __FILE__, __FUNCTION__, __LINE__, true, 0);` you do not have a warning when you compile ? – bruno May 22 '19 at 09:54
  • No.. it compiles fine.. where do you see it is missing arg here ? – Amit Sharma May 22 '19 at 10:05
  • @AmitSharma so your problems are fixed ? Else what is the signature of _MyUtilsLogError ? – bruno May 22 '19 at 10:07
  • Actually I've just snippet/mimicking this code here to formulate the problem I'm facing.. And I don't face any compile issue in my code. `MyUtilsLogError` doesn't have to deal with MyError enums etc.. and just dump some debug info on console. – Amit Sharma May 22 '19 at 10:22
  • @AmitSharma again, do you still have problems or not ? – bruno May 22 '19 at 10:23
  • yes.. it does report MISRA violation as mentioned in question :/ – Amit Sharma May 22 '19 at 10:24
  • @AmitSharma so you have to edit your question to give more, currently we do not have enough to understand your problem. Again what is the signature of _MyUtilsLogError_ and does it knows the enum ?? – bruno May 22 '19 at 10:25
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/193765/discussion-between-amit-sharma-and-bruno). – Amit Sharma May 22 '19 at 10:41
-1

I could solve issues with below approach:

Issue [1]: misra_c_2012_rule_10_4_violation: Essential type of the left hand operand "e" (enum) is not the same as that of the right operand "MySuccess"(boolean).

==>Fix: Rule 10.4 violation fix is:

8  #define PROPAGATE_ERROR_FAIL_MY_1(_err) \
9  do { \
10    e = (_err); \
11  + MyError errSuccess = MySuccess; /* This fixes the 10.4 violations */\ 
11    if (e != MySuccess) \
12   { \
13       MY_UTILS_LOG_ERROR(e, __FILE__, __FUNCTION__, __LINE__, true, 0); \
14        goto fail; \
15    } \
16 } while (0)

So holding the MySuccess in separate MyError var helps to fix the answer. But I don't know how this is fixing the issue here ? It seems without holding MySuccess in separate var macro expansion simply places its value before comparison and MISRA catches this as violation.

Issue [2]: misra_c_2012_rule_11_9_violation: Literal "0" shall not be used as null pointer constant.

==>Fix: Rule 11.9 violation fix is:

- 13 MY_UTILS_LOG_ERROR(e, __FILE__, __FUNCTION__, __LINE__, true, 0); \
+ 13 MY_UTILS_LOG_ERROR(e, __FILE__, __FUNCTION__, __LINE__, true, ""); \

This fixes the issue because MyUtilsLogError() function expects format: arg as pointer to a const char (const char*). So we shouldn't pass 0, passing "" for format arg fixes the Rule 11.9 violations.

Amit Sharma
  • 1,987
  • 2
  • 18
  • 29
  • This doesn't answer the question (which can't be answered since the code doesn't compile). If the warning says "literal 0 shall not be used as a null pointer constant", then you can't just aimlessly replace it with an empty string literal, what does that have to do with anything? You changed the meaning of the code. You should be using NULL. Overall, you are apparently just taking wild chances without knowing what you are doing. – Lundin May 23 '19 at 14:35
  • @Lundin I agree, we can use NULL too to fix this warning... but using `""` is immutable const string and it is OK also to use here ? Can you please explain how I changed the meaning of cide ? I feel using passing `""` to const char* is fine ? – Amit Sharma May 23 '19 at 14:53
  • I have no idea what the function does, so I have no idea. – Lundin May 23 '19 at 14:54
  • @Lundin `format` string is format specifier here as we use in printf() functions.. – Amit Sharma May 23 '19 at 15:02
  • You aren't allowed to use those by MISRA-C. The whole of stdio.h is banned, for very good reasons. – Lundin May 23 '19 at 15:09