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

GCC: Suppress PPCSTATE_OFF invalid-offsetof warnings #12128

Conversation

Dentomologist
Copy link
Contributor

Modify PPCSTATE_OFF and PPCSTATE_OFF_ARRAY macros when using GCC to avoid useless log spam. Specifically, use a consteval lambda with gcc _Pragma statements to disable the -Winvalid-offsetof warning inside the macros.

Each successful build (and many failing ones) on the Android buildbot generates almost 300 cases of -Winvalid-offsetof, resulting in thousands of lines of log spam per build. In addition to bloating the log filesize these spurious warnings make it harder to find actual warnings.

These warnings are generated by calls to the macros PPCSTATE_OFF and PPCSTATE_OFF_ARRAY, which in turn are used by many other macros used by the JIT. The ultimate cause is that offsetof is only conditionally supported on non-standard-layout types, which includes the PowerPCState struct.

To address potential questions of whether there's a better way to handle this:

The obvious solution would be to modify PowerPCState so that it does have a standard layout. This is unfortunately impractical.

To have a standard layout a type can only contain other types with standard layouts. None of the stl containers are guaranteed to have standard layouts, and PowerPCState contains a std::tuple and std::array. PowerPCState also contains a PowerPC::Cache and InstructionCache which themselves contain std:arrays and std::vectors.

Furthermore InstructionCache derives from Cache, and a derived class can only have standard layout if at most one class in its hierarchy has a non-static data member, but both classes have such members. Making InstructionCache have a standard layout would require duplicating all the functionality of Cache so it no longer derived from it, as well as replacing the stl containers. This might require having a raw pointer to said containers, with the manual memory management that implies.

All of that would be much more disruptive than would be justified to get rid of some warnings (however annoying they might be). This is compounded by the fact that PowerPCState hasn't had a standard layout for a long time, if ever, and if the PPCSTATE_OFF macros weren't working reliably it would have become obvious a long time ago.

As to why I picked the lambda solution over other potential changes:

  • Keeping the define as-is and wrapping some gcc #pragmas around it doesn't work because the pragmas don't get included when the define is substituted to the call site.

  • Keeping the define as a non-lambda expression and using inline _Pragma() statements would ideally be better and works fine for msvc, but fails for GCC with "'#pragma' is not allowed here".

  • Turning off -Winvalid-offsetof globally for gcc would work, but there might be other contexts where offsetof is problematic and GCC seems to be the only compiler warning about it.

@JosJuice
Copy link
Member

Doesn't this apply to JitArm64 too?

Modify PPCSTATE_OFF and PPCSTATE_OFF_ARRAY macros when using GCC to
avoid useless log spam. Specifically, use a consteval lambda with gcc
_Pragma statements to disable the -Winvalid-offsetof warning inside the
macros.

Each successful build (and many failing ones) on the Android buildbot
generates almost 300 cases of -Winvalid-offsetof, resulting in thousands
of lines of log spam per build. In addition to bloating the log filesize
these spurious warnings make it harder to find actual warnings.

These warnings are generated by calls to the macros PPCSTATE_OFF and
PPCSTATE_OFF_ARRAY, which in turn are used by many other macros used by
the JIT. The ultimate cause is that offsetof is only conditionally
supported on non-standard-layout types, which includes the PowerPCState
struct.

To address potential questions of whether there's a better way to handle
this:

The obvious solution would be to modify PowerPCState so that it does
have a standard layout. This is unfortunately impractical.

To have a standard layout a type can only contain other types with
standard layouts. None of the stl containers are guaranteed to have
standard layouts, and PowerPCState contains a std::tuple and std::array.
PowerPCState also contains a PowerPC::Cache and InstructionCache which
themselves contain std:arrays and std::vectors.

Furthermore InstructionCache derives from Cache, and a derived class can
only have standard layout if at most one class in its hierarchy has a
non-static data member, but both classes have such members. Making
InstructionCache have a standard layout would require duplicating all
the functionality of Cache so it no longer derived from it, as well as
replacing the stl containers. This might require having a raw pointer to
said containers, with the manual memory management that implies.

All of that would be much more disruptive than would be justified to get
rid of some warnings (however annoying they might be). This is
compounded by the fact that PowerPCState hasn't had a standard layout
for a long time, if ever, and if the PPCSTATE_OFF macros weren't working
reliably it would have become obvious a long time ago.

As to why I picked the lambda solution over other potential changes:

- Keeping the define as-is and wrapping some gcc #pragmas around it
  doesn't work because the pragmas don't get included when the define is
  substituted to the call site.

- Keeping the define as a non-lambda expression and using inline
  _Pragma() statements would ideally be better and works fine for msvc,
  but fails for GCC with "'#pragma' is not allowed here".

- Turning off -Winvalid-offsetof globally for gcc would work, but there
  might be other contexts where offsetof is problematic and GCC seems to
  be the only compiler warning about it.
@Dentomologist Dentomologist force-pushed the gcc_suppress_ppcstate_off_warning_spam branch from 1be4db5 to 58ab94c Compare August 21, 2023 21:01
@Dentomologist
Copy link
Contributor Author

Dentomologist commented Aug 21, 2023

So it does, changed that version too.

The Android shell_1 log is now 871 lines, down from 4,200+.

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess getting rid of all those warnings is worth this bit of ugliness.

@AdmiralCurtiss AdmiralCurtiss merged commit dec2990 into dolphin-emu:master Aug 26, 2023
11 checks passed
@Dentomologist Dentomologist deleted the gcc_suppress_ppcstate_off_warning_spam branch August 26, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants