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

Fix ODR violations discovered when using -flto. #824

Merged
merged 1 commit into from Jan 26, 2022
Merged

Fix ODR violations discovered when using -flto. #824

merged 1 commit into from Jan 26, 2022

Conversation

tsteven4
Copy link
Collaborator

The fedora build shows these:
skytraq.cc:666:8: warning: type 'struct read_state' violates the C++ One Definition Rule [-Wodr]
wbt-200.cc:127:8: note: a different type is defined in another translation unit
brauniger_iq.cc:30:6: warning: type 'state_t' violates the C++ One Definition Rule [-Wodr]
igc.cc:150:6: note: an enum with different value name is defined in another translation unit

However, even with link time optimization I was unable to observe a problem, i.e. testo passes.

There is a relevant note in https://en.cppreference.com/w/cpp/language/definition, see the note
about using unnamed namespaces to resolve this issue.

Also note the use of unnamed namespaces is not recommended in header files,
https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL59-CPP.+Do+not+define+an+unnamed+namespace+in+a+header+file

The fedora build shows these:
skytraq.cc:666:8: warning: type 'struct read_state' violates the C++ One Definition Rule [-Wodr]
wbt-200.cc:127:8: note: a different type is defined in another translation unit
brauniger_iq.cc:30:6: warning: type 'state_t' violates the C++ One Definition Rule [-Wodr]
igc.cc:150:6: note: an enum with different value name is defined in another translation unit

However, even with link time optimization I was unable to observe a problem, i.e. testo passes.

There is a relevant note in https://en.cppreference.com/w/cpp/language/definition, see the note
about using unnamed namespaces to resolve this issue.

Also note the use of unnamed namespaces is not recommended in header files,
https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL59-CPP.+Do+not+define+an+unnamed+namespace+in+a+header+file
@tsteven4
Copy link
Collaborator Author

These are serious issues. With link time optimization I can generate segmentation faults with very similar code (identically named structures of different sizes in multiple translation units). I haven't demonstrated segmentation faults with the released code, but that may be just luck or test coverage.

@tsteven4
Copy link
Collaborator Author

I think we walked into this one with our C to C++ conversion. I am amazed it took this long to surface (actually it was in previous Fedora build logs, but I didn't notice). One motivation for converting formats/filters to classes was to clean up the global namespace.

Note this PR only addresses the identified conflicts. We have lots of other opportunities with structs/enums/... declared in legacy formats, but apparently no other conflicts at the moment. We don't see violations in CI because we don't use link time optimization (-flto).

https://en.cppreference.com/w/cpp/language/definition

If all these requirements are satisfied, the program behaves as if there is only one definition in the entire program. Otherwise, the program is ill-formed, no diagnostic required.

Note: in C, there is no program-wide ODR for types, and even extern declarations of the same variable in different translation units may have different types as long as they are compatible. In C++, the source-code tokens used in declarations of the same type must be the same as described above: if one .cpp file defines struct S { int x; }; and the other .cpp file defines struct S { int y; };, the behavior of the program that links them together is undefined. This is usually resolved with unnamed namespaces.

@GPSBabelDeveloper
Copy link
Collaborator

We'd have had a problem with "unity style" or "allsrc" builds; even in C. "cat [a-z]*> .allsrc.c ; gcc -O .allsrc". It's only really a problem if you go out of your way to stretch the life cycle of The Things across compliation units, as is also done by LTO.

I'd approve of shoving them into an anon namespace, a real namespace, or just using format-local names (which is a kind of namespace: skytraq_status_t).

Technically, I think that posix reserves everything_t for itself, too, but it's become such a common convention that I don't see us - or anyone else - working hard to avoid it. We do avoid leading underscores, but I see that all over, too.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants