I just noticed this code does not even compile: https://github.com/thesofproject/sof/actions/runs/3639665715/jobs/6143338384

Yet Klocwork does not mind...?

Converted to draft, fixing typos and warnings. Pushed in hurry as we are in hurry with release.
Thank @marc-hb for review I'll add asserts and fix it.

Now, I would like to elaborate on following comments:

Compiler error (due to typo) and it's a pointless refactor anyway.

Pointless refactor.

by @paulstelian97

Static analyzers are not always right. Klocwork itself knows this and has a documented process to mark false positives. Use that KW process instead of randomly patching the code.
by @marc-hb

And those are related specifically to this case:

I assume you know what a variable scope is.
I also assume that you understand the difference in C language between "something is working" and "something is working but in language terms is undefined behavior". I also assume, that you know what a "dangling pointer" is.
Ok, lets start from a reference to what return statement does.
I will quote from those docs:

This:

ends the execution of the function

means that it ends function scope.

Now, let's go to the case where return statement returns the expression (emphasis mine):

This means, that the function scope ends with return statement.
Stack frame has additional slot in bytes reserved only for the value returned by the function. Let's illustrate it like so:

This means following:

int func_a()
{
    return func_b();
}
 int func_b()  // THIS IS NO LONGER SCOPE OF A. Expression of func_b is evaluated and return value placed into the stack trace return window.
{
    return X;
}

And more graphically:

This means, that function arguments passed to func_b, if they are pointer types which are copied by value (in C++ there are references whose lifetime is expanded in this situation!), then those are dangling pointers - a pointers to local variables of a func_a that are outside of the scope during execution of func_b.
More graphically:

This is what happens here in:

You pass a global_config->version_fd to ferror function what results in a dangling pointer.
Please undestand that:

int a = ferror(...);
return a;

is not the same as:
return ferror(...);

Compiler might optimize it if ferror is inlined or if ferror has no arguments but also MAY NOT optimize it.
Especially in situaltions like this one.

This specific change would also have the exact same dangling pointer because it literally compiles to the same thing if you have at least -O1...

Also, fact of compiler optimization does not allow developers to break coding rules of the language!

Next time you write something not very kind @paulstelian97 please make sure you know what you talk about.
Same @marc-hb. Please make sure you know what you are saying. Some things are not as trivial as they look.

The problem with your example here is that

int a = ferror(...);
return a;

CAN be optimized to generate the exact same code as

return ferror(...);

The C standard doesn't guarantee any local variable actually gets a stack frame allocation (unless you take its address and that address isn't optimized out for some different reason)

Also... "dangling pointer" I feel like it doesn't mean what you think it means. version_fd is a field of the thing pointed at by "global_config", which doesn't get automatically freed in any way (global_config doesn't point to anywhere on the stack), as such version_fd remains valid (if it's a pointer in the first place anyway). You can't have dangling pointers if the memory they point to isn't freed (and thus remains valid) -- a pointer to valid memory cannot by definition be a dangling pointer.

The return statement ends the execution of the function after the expression has been evaluated (so it knows what value to return).

I challenge you to support everything you claim about C with actual quotes from the standard (or from a public copy of said standard).

Also, fact of compiler optimization does not allow developers to break coding rules of the language!

Well some of the statements you mention are NOT rules of the language (in C specifically -- they would be in other languages but C on purpose leaves some things more vague in order to enable optimizers to be powerful -- for example, you DON'T get a guarantee that local variables are allocated anywhere -- they can be in a register or outright missing if unused)

I do apologize if I come off as rude, but I have to point out that C isn't e.g. Java, where the compiled code does mostly reflect the source code unlike in C where it only really does so with -O0.

@marc-hb

Probably the only reason name could be NULL is because some malloc failed. In that very unlikely case the program should just stop and not print garbage. Please use some kind of assert either here or probably better inside log_vasprintf() (in case KW can understand that)

Default GCC asserts does not work with -NDEBUG flag and I would like it to work with production code when memory allocation fails. Including sof/debug/panic.h fails also since logger does not have architecture defined (its not a platform so...).
Any suggestions on error handling here?

This is a tool, so for error handling you can probably print an error message and call the exit() [EDIT: or abort() might be more appropriate?] function.

I have looked at your past commit history, this is your first time in C code (unless I'm missing some old work -- EDIT: I did find a few basically trivial commits such as a missing cast that did get accepted) on this repo. Sorry for making the "welcome" be so rough; I just have to point out that C doesn't behave pretty much at all like Python and there's a LOT of gotchas that seem to have stumped you when sending this pull request.

I have looked at your past commit history, this is your first time in C code (unless I'm missing some old work -- EDIT: I did find a few basically trivial commits such as a missing cast that did get accepted) on this repo. Sorry for making the "welcome" be so rough; I just have to point out that C doesn't behave pretty much at all like Python and there's a LOT of gotchas that seem to have stumped you when sending this pull request.

@paulstelian97 what are you talking about?
I'm developing MTL firmware for more than a year now in C... I don't know how you checked your commit history.
I guess it had a lot of squashes during upstreams to you know, commit history is not everything. Better not judge people by the book.

I have looked at your past commit history, this is your first time in C code (unless I'm missing some old work -- EDIT: I did find a few basically trivial commits such as a missing cast that did get accepted) on this repo. Sorry for making the "welcome" be so rough; I just have to point out that C doesn't behave pretty much at all like Python and there's a LOT of gotchas that seem to have stumped you when sending this pull request.

@paulstelian97 what are you talking about? I'm developing MTL firmware for more than a year now in C... I don't know how you checked your commit history. I guess it had a lot of squashes during upstreams to you know, commit history is not everything. Better not judge people by the book.

Well I did not see the quality of your code anyway. Still, maybe you're accustomed to embedded compilers which... might be of lower quality than compilers that do userspace Linux (or Windows, they aren't much worse on Windows) programs.

Anyway you claim that some things are meaningful when my experience says otherwise so I would like you to tell me exactly why those things are meaningful.

Anyway you claim that some things are meaningful when my experience says otherwise so I would like you to tell me exactly why those things are meaningful.

I'll gladly elaborate on this on Saturday in my free time since we need to finish release asap and production world will not be happy that I prolong the process due to scientific debates :-)
Let's continue with the review meantime.

Next iteration:

• added name != NULL check using logging and exit() as @paulstelian97 suggested
• added config = NULL assignment to fix errors reported by static analysis tool that reference to local is returned through argument
• removed my retarded attempt to do something with format specifiers to fix format string vulnerability

I guess I'm gonna allow these changes (there's no longer any actively-breaking change, just two changes that prevent crashes and a good chunk of changes that do nothing). They are still meaningless (you trust the tool more than the experts, which is why I can't outright approve these changes) AND have not truly made an attempt to explain why the tool is right over the experts for most of these changes. I will no longer block the merge however.

You are not going to allow anything. Your review had been dismissed and this is not SOF main branch but a product release branch. You are not only maintainer here and I think @lgirdwood should do the review. You should certainly be more careful with what you say and do as a maintainer of this project. Further comments on my developer career I will take as an offence.

I will not make comments on your developer career, that I can agree with. And while I don't maintain this branch, I believe that marc does, and Liam isn't the only maintainer that can deal with these branches.

I'm cool with Intel guys commenting and reviewing patches on i.MX stable branches just fine, and WILL take into account what they are saying. You don't seem to take stuff into account from me OR from marc, so I strongly doubt Liam will be of a different opinion. The only real difference is he'll be more polite than myself in saying most of these changes are either pointless or wrong.

After consultations with team turns out that logger is not part of the release for MTL, closing

I'm closing this PR
after discussions we reailzed that logger is not a tool intended for MTL, so is not a part od MTL-002-stable release

Next time you write something not very kind @paulstelian97 please make sure you know what you talk about.

Unacceptable tone @aborisovich. This is not how we work with partners and colleagues.

Please understand that:

int a = ferror(pointer_to_local_stack);
return a;

is not the same as:

return ferror(pointer_to_local_stack);

This is an interesting C quizz. Please correct me but I think and hope your long and illustrated explanation can be summarized with just these 3 words: "tail call optimization"? I would naively trust compilers to "do the Right Thing" and NOT perform any tail call optimization when the function argument contains any reference to the local stack but maybe that's too naive. C is a terrifying minefield of unintuitive and undefined behaviors so maybe this is yet another one of them? https://stefansf.de/c-quiz/ https://www.bing.com/search?q=linus+torvalds+gcc, etc.

Also, maybe most compilers are smart enough but a few are not (or they are buggy) and Klocwork is being very conservative and erring on the "false positive" side? It would suit its role.

I'm also a bit sceptical that a (too?) simple intermediate variable trick is enough to defeat tail call optimization, now that seems a bit too naive too?

Anyway config is NOT a pointer to the local stack in most (all?) cases found by KW so these were definitely false positives. As noted by @paulstelian97, nothing is ever dangling here because it always points to something in a higher scope.

I see sof-logger is removed in that branch by #6754 and that's OK but you still need to fix the KW process because there will always be other false positives that must be disposed (after very careful team review) and it won't always be possible to change or delete the code to avoid them.

Static analyzers always produce false positives which is why they all have a process to ignore some warnings and KW is no exception.

I'll gladly elaborate on this on Saturday in my free time since we need to finish release asap and production world will not be happy that I prolong the process due to scientific debates :-)

I'm looking forward to Saturday as long as we all focus 100% on the technical questions and stop all personal comments! https://www.sofproject.org/community/

Interesting experimentations:

https://quuxplusone.github.io/blog/2021/01/09/tail-call-optimization/

tl;dr: some compilers perform tail call optimizations but they stop when things "get too complicated", likely out of fear of breaking things (as usual with most optimizations and undefined behaviors)

https://godbolt.org/z/vcY3v9 - See assembly generated "live" by a few compilers

BTW different static analyzers find different bugs and the CMake maintainer in Zephyr has submitted a pull request to support any of them generically:

• cmake: implement build infrastructure for supporting SCA tools. zephyrproject-rtos/zephyr#52671

This is very promising, looking forward to it!

Different static analyzers also emit different false positives that may conflict with each other: write the code one way and analyzer 1 complains, write it differently and now analyzer 5 complains! "Static analyzer whack-a-mole" gets nowhere fast. It's OK to simplify the code to help all analyzers (and humans!) understand it. It's not OK to make it more complicated to silence a single analyzer. That's why many static analyzers offer a way to filter out false positives (after very careful team review).

Fixes submitted to the main branch by @kv2019i (thanks!)

• logger: exit with error if calloc fails #6844,
• sof-logger: print error if -u uart option is given with no infile #6846,
• sof-logger: exit with error if malloc fails #6847,
• sof-logger: ensure NULL is not passed to printf/fprintf #6848

Default GCC asserts does not work with -NDEBUG flag and I would like it to work with production code when memory allocation fails. Including sof/debug/panic.h fails also since logger does not have architecture defined (its not a platform so...).
Any suggestions on error handling here?

Good catch. abort() is a good alternative, see Linux demo in #6848 (comment)

exit(non-zero) is good when failing input sanitization or other situations when the program is still "in control". Out of memory is an "out of control" situation (that will practically never happen with sof-logger)

Making global_config truly global with a minimal number of lines changed in:

• logger: make "global_config" truly global #6862