Skip to content

Conversation

@theomagellan
Copy link
Contributor

Implements the Wpadded diagnostic for clang-cl.
#61702

@theomagellan
Copy link
Contributor Author

theomagellan commented Apr 4, 2025

Fixes the issue in #130182 where the variable RemainingBitsInField was used uninitialized, leading to undefined behavior in certain cases.
Although I wasn't able to reproduce the undefined behavior using Clang 18.1.8, I validated the fix with Valgrind while running the windows-Wpadded.cpp test as described here #130182 (comment).

@mikaelholmen @asb, could you please confirm if this resolves the issue on your end?
Thanks for your patience, and I hope the pin isn't too disruptive.

return;
}
unsigned CharBitNum = Context.getTargetInfo().getCharWidth();
uint64_t DataSizeInBits = Context.toBits(DataSize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand all the implications of using DataSize... yes, it handles empty structs, but there's also the interaction with RequiredAlignment, which I'm not sure behaves the way you want for non-empty structs. (I think you can test this using an "aligned" attribute on a struct.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right, using DataSize did create issues while using certain alignment attributes. I added some in the test file.
Now, 1 byte is artificially added to UnpaddedSizeInBits when Size is zero before aligning the empty struct to its alignment. This allows to keep the checks as they used to be (using Size instead of DataSize) and also takes care of the empty struct case.

Thank you for catching that case!

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@efriedma-quic efriedma-quic merged commit 41892fc into llvm:main Apr 14, 2025
12 checks passed
@DKLoehr
Copy link
Contributor

DKLoehr commented Apr 15, 2025

After this change, it seems that -Wpadded is included in -Wall for clang-cl only, but not clang or clang++. I'm not sure why, but the inconsistency seems strange, is it intentional?

(Reproduction: I compiled the sample program in #61702 using all three executables, with and without -Wall; only clang-cl.exe -Wall produced this warning)

@efriedma-quic
Copy link
Collaborator

For -Wall, see #102982

@DKLoehr
Copy link
Contributor

DKLoehr commented Apr 15, 2025

Got it, thanks.

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.

3 participants