Build fails with strict-aliasing violations #12675
Comments
|
This is a slightly different issue than, but directly related to #12674. See #12674 (comment) for further details. Thanks for the report! |
|
@jsquyres in this case, the UB is far more egregious. Code such as routinely breaks even on current versions of GCC and Clang, and I'd seriously consider fixing this |
|
In particular, type punning is legal to do, but you have to do it "properly". e.g. memcpy is superior to casting and typically generates the same code as one wanted the cast to produce. This is the kind of thing that routinely breaks because you did -O2, not because of -flto, but -flto simply causes it to break "even more often". As I noted in the OP:
and it still merits fixing regardless of LTO. Maybe I shouldn't have mentioned LTO in the first place. |
|
@SoapGentoo Your example is incomplete; what exactly is |
|
Looks like it is ompi/ompi/mca/common/monitoring/common_monitoring.h Lines 73 to 87 in a24343d |
|
Please let's avoid having this discussion again here. We use pointers to objects with well defined alignment attribute to hide additional information. In this particular instance, the bit 0 signals if this remote proc structure has been expanded or not: |
|
Again, this is totally unrelated and misdiagnosed as being related. This has nothing to do with function signatures, nor with changing the function signatures of the API/ABI. LTO imposes additional unrelated requirements above and beyond being strict-aliasing clean -- whereas this is about miscompilation with -O2. The C standard allows you to convert a pointer via type punning. It just says you aren't allowed to use a cast for it, and recommends using memcpy for your type punning needs instead. Sometimes, a cast for something that cannot validly alias will produce garbage code due to UB. In the cases where it does what you actually wanted it to do, a memcpy compiled by a good compiler such as GCC produces the same machine code as the cast. memcpy is guaranteed to avoid UB. GCC guarantees your memcpy is efficient. ... The code in question here is completely inside an internal routine, and changing the mechanism you use for type punning won't affect code outside that function. |
|
Let me give you a simple example why your code is broken here: In notice how bar returns |
|
I believe much of the issue here has been due to miscommunication, at least for this poor mortal. I realize you folks spend many hours/days talking regularly amongst yourselves about these things and have therefore developed a clear understanding of the context surrounding these issues you are trying to raise, but I certainly haven't - and I suspect many of the rest of us are in a similar situation. Simple example: it took me a few readings to finally figure out that your "UB" didn't stand for "Upper Bound", which is the most common meaning for that acronym. Likewise, I am still trying to fully grok your use of "OP" - which I think means "Other People" as opposed to "OutPatient" (as the medical community has adopted)? Not entirely sure. If you are going to use acronyms freely in your discussion, it helps to at least define them once early on to ensure others understand what you are saying - it's why we require you to do so in, for example, publications. So after going back and re-reading all the exchanges between the several issues spanning two projects, here is the gist I've tried to assemble. This entire discussion appears to have been about difficulties in optimizing code. Optimizers have problems correctly interpreting various historically-permitted C operations, and your are saying that the C standard has tried to evolve to make it easier for the optimizers to succeed. This I can grok - over the 50+ years I've been programming, optimizers have always had difficulty ensuring that the resulting opimized code actually resulted in the original behavior. Most commonly, cranking up the So to summarize, the issue here isn't that some of the code you've referenced isn't "defined" - those code patterns have been allowed by the C standard since its inception and are widely used. What you are raising is that some (many?) optimizers have problems properly parsing those patterns into something that preserves the original result - in other words, they may result in faster code, but the resulting behavior isn't what the programmer intended. You at least imply that the C standard has been modified to perhaps not necessarily forbid them - after all, compilers continue to accept those code patterns, and I rather doubt that is going to change anytime soon as it would break a lot of existing code - but to at least recommend alternative approaches. Hence, you are suggesting that we utilize different coding patterns that (a) produce the same resulting behavior, but (b) are easier for optimizers to parse while enhancing the likelihood of getting the same behavior out of the optimized code. Correct? If so, then that is useful advice. I confess that the programming community used to take the position of insisting that the problem lies in the optimizer and not the original code, and so improving/fixing the optimizer is the correct response. However, optimizer developers may have simply "hit the wall" and convinced the C standard community to help by modifying the standard - not all of us watch every move of that body. Could well have missed something, especially if the compilers don't reflect those changes (which, if I'm understanding your comments, they clearly don't at this time since we are not seeing warnings or errors generated by those code patterns across a very wide range of compiler families and versions).
I personally find these kind of statements disturbing. You imply that GCC is always a "good" compiler,, therefore implying that other compilers are not - but that is an opinion not necessarily universally shared, especially across all versions of GCC. Basing an entire project on what someone thinks about GCC vs clang and other compilers would be irresponsible. I think this argument would be more convincing if one could provide a guarantee that "memcpy produces the same machine code as the cast across all optimizers and is at least as efficient as the cast". For people where nanoseconds matter and whose projects must support all compilers, these are concerns. If all we are doing is making things easier for GCC and some subset of optimizers, then that isn't terribly convincing - especially given the magnitude of the proposed code disruption. |
|
@rhc54 you're 90% there. Let's take the simplest example: This is probably the simplest case of undefined behavior. Once you encounter undefined behavior, all bets are off and compilers can do whatever they like. This has been the case since the first C standard (C89). Historically, this code would usually segfault (in a hosted environment) but in this day and age, I have seen compilers optimize out the null-pointer dereference (and the Back in 1989, compilers were a simpler thing, and only with stuff like SSA and deeper optimization passes did compilers manage to tease out more performance from code. Here's another example: when compiling with compiling with what's the problem with this code? Signed (but not unsigned!) overflow is undefined behavior, and has been undefined behavior since the very first C89 standard. You're starting at a very large positive value ( The UB we're discussing here is around type punning. Historically, people have treated C as "portable assembly", which it never was. People assumed they could take a Compilers implement the C standard to the word, and most C developers have never read the actual C standard. The example I have shown in #12675 (comment) is an example of UB where type punning through pointer casts leads to a subtle logic bug in 10 lines of code. As distro maintainers, we are sensitive to these issues, since we have to debug this sort of code every time a new GCC/Clang major version comes out. |
|
I would certainly agree that dereferencing a pointer that went from float* to uint32_t* is invalid - one can clearly see that is a mistake. I'll set aside the "portable assembly" comment - I recall sitting through some discussions where that definitely was the focus at the time, but it's somewhat irrelevant here. I think looking deeper into the code for issues is a fine thing, but we have to give people some means for doing so. I've complained about the large volume of warnings OMPI currently (and historically) generates for quite some time, to little avail as the community hasn't seen corresponding failures and/or performance issues (and people are motivated by failures and performance). In PMIx and PRRTE, I set So motivation and means will be issues that merit some thought. If I correctly grok your comments, it would appear that you would like us to add optimized compiles to the standard testing to try and tease out more of these issues? Is that a correct statement? I know my automated testing does not include Do you have any suggestions on how we might better surface the problems you raise? The code base is too large for manual review, and if the compilers don't flag the problems, then they won't be found. Is running the code thru |
|
My apologies. OP is a somewhat common term in online forums, which GitHub issues could be considered to be, for "Original Post". UB is an acronym for an ISO C standards document term, "Undefined Behavior".
Not entirely, but similar. Optimizers have problems with "doing what you mean" rather than "doing what you said". The C standard hasn't evolved at all -- the compilers do exactly what the ISO c99 standard text permits them to do, if you compile with -std=c99, but what has changed is that users want compilers to "be better" and "generate faster programs" and compiler authors have obliged by redesigning their compilers to draw more and more inferences about what your code might have meant, and frequently this means pedantically reading the standards text and taking advantage of various forms that the standard has always said is Undefined Behavior to outright delete code that the compiler thinks cannot ever be reached. Which leads us to...
Improving the optimizer here isn't an option. The optimizer developers are, correctly, stating that they are allowed by the original published text of the standard to do what they're doing today, and that the current situation is what "improving the optimizer" means, because they have improved it to generate faster code, all the time.
Sorry, to clarify what I mean is that gcc is an example of a good compiler, and so is clang -- I don't happen to personally use clang so I didn't happen to mention it. I have some skepticism that tinycc is very good, but on the other hand I don't think anyone alive uses tinycc because they think it's good -- rather because they think it is tiny and they like tiny things. I'm also skeptical about Microsoft's MSVC, in fact I recall someone complaining that MSVC generated several machine code instructions rather than one by using the memcpy approach. But fortunately openmpi doesn't care about that because Windows support was dropped over a decade ago and even if it hadn't been, using MSVC from autotools is kind of painful and using a mingw compilation environment with choice of gcc or clang is pretty common for Windows. |
I don't believe a 0% false positive rate is achievable - if the community decides to pursue this, then they will have to accept some need for triage of the results. Issue this creates is that you cannot use it in CI else we will never pass another PR. The community will have to work out the logistics - perhaps a "non-required" CI would suffice, though that means someone has to look at its output before deciding "pass/fail". Extra burden, may not be acceptable.
I'll leave that for the OMPI community to debate - I can see some of their point too, but it is up to them.
Worth at least giving it a smoke test to see (a) what it catches in the current code, and (b) what problems it creates. I've only had light experience with ASAN (none with UBSAN) and it has had mixed results. Still, worth taking a look at it. Thanks! |
So having the optimizer output garbage is acceptable to the optimizer developers because "it is faster garbage"? I find it very hard to believe that the community would behave that way. If that truly is their attitude, then perhaps we should discourage people from turning them on 🤷♂️ Seems to me that a better approach would be for the compiler, optimizer, and user community work together to ensure that optimized code is in fact correctly reproducing the intended behavior. |
|
A 0% false positive rate is absolutely achievable. It took us a year, but our corporate codebases have ASAN and UBSAN hits. In fact, I don't ever recall a single ASAN or UBSAN false positive in the 10 years I've been using it (which is also a declared goal). That said, there are still a bunch of false negatives and I have observed these.
have you read the GCC bugreports I linked to? The problem remains: most C programmers don't know C, but dangerously think they do. |
Their viewpoint is that "this is the Garbage In, Garbage Out principle in action -- simply write C code that is valid according to the original, unchanged published text of the standard, and it won't output garbage".
That would be a "Do What I Mean compiler". I heard that chatgpt is working to solve the age-old programming task of figuring out what "meaning" means. Compilers don't currently include an AI component, and try to work with what you said, rather than what you meant. Considering the accuracy problems with AI today, I'm relieved. ... That being said, there's only two communities here: the compiler (which is the optimizer), and the users. The user community has spent a decade asking the compiler developers to make a faster compiler... |
Depending upon the precise flags you set, and how willing you are to not check for certain problem types. Your example is focused on the one thing you are currently concerned about, but I am sure I can find others who would just as ardently point to some other issue they care about.
I fail to understand how this relates to what I said - I was talking about "the optimizer developer community" and them not caring about generating garbage code. I think your comment about C programmers is rather prejudicial and somewhat derogatory, so let's perhaps leave such opinions unsaid in the future? |
Okay, I've had enough - this has degenerated into something sad. I'll think about some of the good things in the thread and try to ignore where things have fallen off the track. Adios. |
|
Hi, I just want to clarify on this. In general, https://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html is a superb read on "why does the compiler produce bad output if it knows the input is bad", written by the creator of llvm.
The reason is that optimizers do not know that their input is garbage, nor that their output is garbage wrt. the intent of the programmer. Optimizers do not operate on the semantics of the programming language, but on their own internal representation (IR).
They do, which is why diagnostics like |
Background information
What version of Open MPI are you using? (e.g., v4.1.6, v5.0.1, git branch name and hash, etc.)
5.0.3 tarball
Describe how Open MPI was installed (e.g., from a source/distribution tarball, from a git clone, from an operating system distribution package, etc.)
Using Gentoo Portage (I am currently working to upgrade Gentoo from openmpi 4.1.6 to 5.0.3).
Details of the problem
I... had originally tried to build with the following *FLAGS to optimize the build:
-flto=4 -Werror=odr -Werror=lto-type-mismatch -Werror=strict-aliasingThe -Werror=* flags are important to detect cases where the compiler can try to optimize based on assuming UB cannot happen, and miscompile code that has UB in it. strict-aliasing issues are always bad but LTO can make them even worse.
Due to #12674 I cannot configure with LTO. I tried disabling LTO to see if there are any more errors that need to be fixed in parallel to the configure issue.
I got this error:
There are actually a lot more like this. Github won't let me post them all because "There was an error creating your Issue: body is too long, body is too long (maximum is 65536 characters)." Heh. ;)
The remaining errors can be found here: Full build log: build.log
The text was updated successfully, but these errors were encountered: