Skip to content
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

Open
gisburn opened this issue Jul 2, 2024 · 2 comments
Labels
backburner Low priority (but feel free to fix it and do a PR) TODO Things to be done before releasing

Comments

@gisburn
Copy link

gisburn commented Jul 2, 2024

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.

@McDutchie McDutchie added the TODO Things to be done before releasing label Jul 2, 2024
@McDutchie
Copy link

McDutchie commented Jul 2, 2024

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 -fno-strict-aliasing but hopefully they will support -std=c90 which should also solve it.

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.

@McDutchie McDutchie added the backburner Low priority (but feel free to fix it and do a PR) label Jul 24, 2024
@McDutchie
Copy link

McDutchie commented Aug 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backburner Low priority (but feel free to fix it and do a PR) TODO Things to be done before releasing
Projects
None yet
Development

No branches or pull requests

2 participants