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

[BUG] fails to compile on windows with clang-18 / clang-19 #1877

Open
kelbon opened this issue Nov 13, 2024 · 5 comments
Open

[BUG] fails to compile on windows with clang-18 / clang-19 #1877

kelbon opened this issue Nov 13, 2024 · 5 comments

Comments

@kelbon
Copy link

kelbon commented Nov 13, 2024

  // Offset tests to ensure commonly accessed data is on the first cache line.
  const int cache_line_size = 64;
  static_assert(offsetof(State, error_occurred_) <=
                    (cache_line_size - sizeof(error_occurred_)),
                "");

error:

/src/benchmark.cc:191:17: error: offset of on non-standard-layout type 'State' [-Werror,-Winvalid-offsetof]
  191 |   static_assert(offsetof(State, error_occurred_) <=
      |                 ^               ~~~~~~~~~~~~~~~
E:\llvm_\LLVM\lib\clang\18\include\__stddef_offsetof.h:16:24: note: expanded from macro 'offsetof'
   16 | #define offsetof(t, d) __builtin_offsetof(t, d)
      |                        ^                     ~
1 error generated.
@kelbon
Copy link
Author

kelbon commented Nov 13, 2024

Please, remove Werror flag from build, because new warnings / new enabled warnings broke compilation

@dmah42
Copy link
Member

dmah42 commented Nov 13, 2024

thanks for the report. which compiler/OS are you on? and which version of benchmark are you building?

the warnings/errors exist to provide information about possible mistakes, so removing it doesn't really help. i'd rather take the opportunity to fix the problem. if folks read warnings we could totally rely on them but...

@dmah42
Copy link
Member

dmah42 commented Nov 13, 2024

in this case, we're getting burned (finally) for using offsetof on a data structure that is very much not standard layout: it has non-static data members with different access control.

we could remove the static_assert and replace it with a well-placed comment to not move things around in State but obviously that's less safe than what we have. we could also disable the warning, but it's not obvious to me how big a deal it is... does it make the static_assert invalid?

@kelbon
Copy link
Author

kelbon commented Nov 13, 2024

thanks for the report. which compiler/OS are you on? and which version of benchmark are you building?

the warnings/errors exist to provide information about possible mistakes, so removing it doesn't really help. i'd rather take the opportunity to fix the problem. if folks read warnings we could totally rely on them but...

I used 1.71 on windows with clang-18/19, it seems to be fixed in 1.9.0, but i dont think its 'invalid' warning

#1821

@dmah42
Copy link
Member

dmah42 commented Nov 15, 2024

yes i'm not sure it's invalid either, but this at least unblocks us until this becomes an error.

i'm open to hearing ideas for how to capture the spirit of the check in another way.

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

No branches or pull requests

2 participants