-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[portability] Make AST code aliasing clean and use -fstrict-aliasing with ISO C17 aliasing rules #760
Comments
But this code base is written in C90 (although it does conditionally use a couple of features from later standards, subject to iffe feature tests). I don't know what the "many" modern compilers are that don't support By the way, I don't fully understand strict aliasing rules yet. I've tried to learn about them, but the information I can find via Google seems to consist mostly of a lot of hand waving and precious little in the way of actual information. If anyone has any good quality pointers for learning about them, I'd appreciate that. However, one thing I have learned over the years of working with this code base is that, just because a compiler throws a warning, does not mean it's actually a problem. I have seen far too many spurious warnings to take them at face value. Many/most of the warnings that were thrown before @JohnoKing added the -fno-strict-aliasing flag said that this or that may violate strict aliasing. But I don't actually know if they really do or not. I am not opposed to making this code compliant with strict aliasing, but I also don't want to spend a lot of time on it myself. All of my available time and energy is taken up fixing ksh bugs. I'd like to get around to developing some new features but even that is not happening, because I keep finding more bugs. So, if you feel strongly that this should happen, I'm afraid you are going to have to put in the work, or get someone else to put in the work. I would be happy to review patches and PRs. |
Finally found a straightforward answer: yes, assuming common initial struct members for different struct types is UB. We need a union for that. https://stackoverflow.com/questions/57411165/casting-structs-and-common-initial-sequence So, that's yet another way in which this code base is chock-full of UB because it does this all the time. |
All the commonly used and supported compilers (IBM XL C/C++, IBM Open XL C/C++, GCC, Clang, NVIDIA/Portland Group, Oracle Studio, Intel C) have options to control the use of strict-aliasing optimizations. There are some compilers that don’t, but they also don’t take advantage of strict-aliasing optimizations (like DMD ImportC or MSVC). I’m unsure of what the status is with HP-UX (acc) and Cray compilers. I imagine that any system will have at least one of the above compilers available, and if not—while not ideal—ksh could still be built with such a compiler with optimizations disabled to avoid the issue. |
I have recently done some performance tests on a few of my own code bases, and have found that enabling strict-aliasing optimization with GCC 14 improves performance by 4-5% on average, so there is a real benefit to it beyond just correctness. GCC supports an intermediate optimization ( |
Make AST code aliasing clean and use -fstrict-aliasing + -Wstrict-aliasing=3. Many mondern compilers don't have something like -fno-strict-aliasing, and just use ISO C11/C17 aliasing rules.
The text was updated successfully, but these errors were encountered: