fixed #12108 - Crash in CTU::FileInfo::getErrorPath() with Clang-built binary
#5746
Conversation
|
I have not yet reduced the issue to report it upstream. I assume it is some ODR violation-ish issue with identically named classes which use internal linkage. |
firewave
changed the title
CTU::FileInfo::getErrorPath() with Clang-built binary
|
so anonymous namespace is not anonymous? ouch!! |
|
I wonder if some release packages are built with clang. It could. I don't think we know. If we knew that no release packages would be built with clang I would prefer to not add this hack here. They should just fix it in Clang. :-( |
It's not just distros. I ran into this as Clang is my preferred compiler and pfultz2 also reported it. I would also prefer not to add the hack but that's where we are at now. I will simply it with an alias though and make it conditional. So it looks less awkward. It also lacked a comment why it was done.
Well, I need to file a ticket first. Still working on reducing it. |
| // a Clang-built executable will crash when using the anonymous MyFileInfo later on - so put it in a unique namespace for now | ||
| // see https://trac.cppcheck.net/ticket/12108 for more details | ||
| namespace | ||
| #ifdef __clang__ |
There was a problem hiding this comment.
hmm don't need to put a ifdef here.. we could put some such code in another location:
#ifdef __clang__
#define CLANG_NAMESPACE(NAME) NAME ## _internal
#else
#define CLANG_NAMESPACE(NAME)
#endif
And then we can use it like so:
namespace CLANG_NAMESPACE(CheckBufferOverrun) {
Feel free to invent better names. What do you think?
There was a problem hiding this comment.
hmm .. that does not solve the "using ...".. then we one more macro for that also :-(
There was a problem hiding this comment.
It is just a temporary very local workaround. We should not overthink it.
There was a problem hiding this comment.
sounds good that it's temporary.. however when would it be safe to get rid of this? I assume you used some released clang binary. does it affect many versions?
There was a problem hiding this comment.
I don't know yet.
There was a problem hiding this comment.
Maybe this is actually invalid code/UB which simply isn't detected by anything yet.
interesting! Very good catch if you find it :-)
There was a problem hiding this comment.
Can we merge this now? This is a release blocker.
There was a problem hiding this comment.
If I only move one of the object back into the anonymous namespace it works.
If we are using an anonymous namespace then it shouldn't be ODR violation(as they are different symbol names). Probably something is wrong in the linker(or a combination of clang and gnu linker since its only a problem on linux and not mac).
There was a problem hiding this comment.
Can we merge this now? This is a release blocker.
Yea I think we should merge this for the release otherwise this cant be built with clang on ubuntu.
There was a problem hiding this comment.
If we are using an anonymous namespace then it shouldn't be ODR violation(as they are different symbol names). Probably something is wrong in the linker(or a combination of clang and gnu linker since its only a problem on linux and not mac).
Good point. I want to break down my upstream report in a few days and check the generated symbols. GCC is generating more in a binary than Clang but on that level there is still too much noise in the output.
Feel free to chime in the upstream discussion (if any should occur).
|
I cherry-picked that change into #5744 and the builds were successful: https://github.com/danmar/cppcheck/actions/runs/7169277973 |
lib/checkclass.cpp
Outdated
| @@ -3438,6 +3444,10 @@ namespace { | |||
| }; | |||
| } | |||
|
|
|||
| #ifdef __clang__ | |||
| using CheckClass_internal::MyFileInfo; | |||
There was a problem hiding this comment.
You could use an inline namespace so you don't need the using statement.
There was a problem hiding this comment.
This is a like 5 line workaround so we do not ship with a crash. All this time discussing this is taking away from the time I could spend to produce an example to actually report upstream. And it prevents me from fixing other things as this blocks other PRs...
There was a problem hiding this comment.
Thanks for pointing that out. But as I said before - it is just a workaround and we should not overdo it. May initial approach was just bad but I think the current code is okay enough to merge. After all this might just be deleted at some point.
I also wonder if we properly handle inline namespaces in our checks. Yet another rabbit hole to follow.
|
I finally reported it upstream: llvm/llvm-project#75275. |
This is actually just a workaround as it seems the issue is an upstream Clang one.