Add -Wpacked-non-pod to -Wall

[SlaterLatiao]

Summary

[clang][diagnostics] Adding -Wpacked-non-pod to -Wall

• Users will be informed when non-POD is not packed using -Wall. This is also consistent with GCC.
• Fixes PR#60832.

[dblaikie]

Yeah, looks consistent with GCC ( https://godbolt.org/z/EGc3Ec9YT ), as you say - so I'm OK with it. But wouldn't mind a second opinion.

[SlaterLatiao]

Test -Wpacked-non-pod not emitting warnings of "packed attribute is unnecessary".

[cjdb]

Thanks for working on this!

[denik]

clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp

4

This one is a very neat test. I think it does what we want :)

5

Probably we don't need this one. But I'm not sure if there is any impact of the abi on`packed-non-pod`. According to the original test it checks only -Wpacked packed attribute is unnecessary warning.

[SlaterLatiao]

• Removed the -triple flag from warn-padded-packed testcase.

[SlaterLatiao]

clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp

5

I added this line to make sure all the packed attribute is unnecessary warnings in the existing test of -Wpacked are not reported by -Wpacked-non-pod. Yeah if the abi doesn't have any impact on -Wpacked-non-pod this test is unnecessary.

[aaron.ballman]

Please be sure to add a release note for the change, and it'd probably be good to have a test case that shows this triggers under -Wall (the modified test case explicitly names the diagnostic).

[dblaikie]

clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp

5

the old ABI shouldn't emit the -Wpacked-non-pod warning since the warning isn't correct in the old ABI - where packed is applied to non-pod equally as pod.

[SlaterLatiao]

In D151162#4369584, @aaron.ballman wrote:

Please be sure to add a release note for the change, and it'd probably be good to have a test case that shows this triggers under -Wall (the modified test case explicitly names the diagnostic).

Thank you, Aaron. I'll add the test case. Should this change go under Improvements to Clang’s diagnostics in the release note?

[aaron.ballman]

In D151162#4369717, @SlaterLatiao wrote:

In D151162#4369584, @aaron.ballman wrote:

Please be sure to add a release note for the change, and it'd probably be good to have a test case that shows this triggers under -Wall (the modified test case explicitly names the diagnostic).

Thank you, Aaron. I'll add the test case. Should this change go under Improvements to Clang’s diagnostics in the release note?

Yes, that sounds like a good place for the note. Thank you!

[SlaterLatiao]

• Test behavior of -Wpacked-non-pod in -Wall and update test case name.
• Update release note.

[SlaterLatiao]

• Fix missing parenthesis.

[aaron.ballman]

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like us to use for patch attribution?

[denik]

Thanks @SlaterLatiao!

[SlaterLatiao]

In D151162#4373027, @aaron.ballman wrote:

LGTM! Do you need someone to commit on your behalf? If so, what name and email address would you like us to use for patch attribution?

Thank you, Aaron! Please use Zenong Zhang and [e-mail] for the commit.

[aaron.ballman]

Unfortunately, I had to revert this in 53333c8d44d690f6e8fd6f31803aecb17329cdb5 due to a bot breaking: https://lab.llvm.org/buildbot/#/builders/245/builds/8876

[SlaterLatiao]

I shouldn't remove -triple=x86_64-none-none in the test case. The "padding struct 'S1' with 1 byte" warning can be different in different architectures. I'll add it back.

[SlaterLatiao]

Adding back the -triple flag

• The padding size can be different on different architectures.

clang/include/clang/Basic/DiagnosticGroups.td

 
 // Uniqueness Analysis warnings
 def Consumed       : DiagGroup<"consumed">;
 
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as
 // DefaultIgnore in addition to putting it here.
 def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool,
-                            MisleadingIndentation]>;
+                            MisleadingIndentation, PackedNonPod]>;
 
 // Warnings that should be in clang-cl /w4.
 def : DiagGroup<"CL4", [All, Extra]>;
 
 // Warnings enabled by -pedantic.  This is magically filled in by TableGen.
 def Pedantic : DiagGroup<"pedantic">;
 
 // Aliases.

clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp

 // RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,top %s -emit-llvm-only
 // RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,abi15 -fclang-abi-compat=15 %s -emit-llvm-only
+// -Wpacked-non-pod itself should not emit the "packed attribute is unnecessary" warnings.
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpacked-non-pod -verify=top %s -emit-llvm-only
+// -Wall should not emit the "packed attribute is unnecessary" warnings without -Wpacked.
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wall -verify=top %s -emit-llvm-only
+
 
 struct S1 {
   char c;
   short s; // expected-warning {{padding struct 'S1' with 1 byte to align 's'}}
   long l; // expected-warning {{padding struct 'S1' with 4 bytes to align 'l'}}
 };
 
 struct S2 { // expected-warning {{padding size of 'S2' with 3 bytes to alignment boundary}}

clang/test/Misc/warning-wall.c

 CHECK-NEXT:    -Wbitwise-op-parentheses
 CHECK-NEXT:    -Wshift-op-parentheses
 CHECK-NEXT:    -Woverloaded-shift-op-parentheses
 CHECK-NEXT:    -Wparentheses-equality
 CHECK-NEXT:    -Wdangling-else
 CHECK-NEXT:  -Wswitch
 CHECK-NEXT:  -Wswitch-bool
 CHECK-NEXT:  -Wmisleading-indentation
+CHECK-NEXT:  -Wpacked-non-pod
 
 
 CHECK-NOT:-W