[Clang] Fix missing diagnostic for non-standard layout type in `offsetof` #65246

kasuga-fj · 2023-09-04T05:58:04Z

Clang warns diagnostic for non-standard layout types in offsetof only if they are in evaluated context. With this patch, you'll also get diagnostic if you use offsetof on non-standard layout types in any other contexts

cor3ntin · 2023-09-04T06:37:13Z

Thanks for working on this.

This generally looks good to me.
Can you add an entry in clang/docs/ReleaseNotes.rst?
Thanks!

cor3ntin · 2023-09-04T07:07:53Z

Thanks. LGTM.
I'd like to do things properly so I'll wait for CI to be fixed before approving, hopefully we can figure that out today!
(Nothing to do on your end)

kasuga-fj · 2023-09-04T07:08:57Z

Thank you for your quick review!

clang/test/SemaCXX/ms_struct.cpp

tbaederr · 2023-09-04T11:21:43Z

Okay, makes sense to me. Should probably also be backported to 17.

cor3ntin · 2023-09-04T11:36:40Z

@tbaederr We are getting rather conservative as to what gets backported. I think that does not meet the bar.
Maybe for 17.0.x

metaflow · 2023-09-04T13:08:46Z

@kasuga-fj could you please re-upload change to trigger the build?

tbaederr · 2023-09-04T13:10:28Z

@cor3ntin Yes, I wrote this in the wrong PR 🤦

kasuga-fj · 2023-09-05T01:25:07Z

@metaflow I added an empty commit and the build was triggered, but it still failed. Could you help me?

hazohelet · 2023-09-05T02:12:30Z

FWIW, an empty commit didn't trigger CI correctly yesterday, so pushing a non-empty commit might fix the CI issue.

kasuga-fj · 2023-09-05T02:26:23Z

Ah, thanks. Then what should I do? Can I make another commit with the same content as the last commit and force push? (sorry, I don't fully understand the LLVM's PR rules)

hazohelet · 2023-09-05T02:47:47Z

Adding an irrelevant new line in a modified test file would suffice to trigger the CI build.
Force-pushing doesn't seem to be desirable during code review.

metaflow · 2023-09-05T04:43:17Z

alright, build has been triggered successfully! thank you <3

kasuga-fj · 2023-09-05T08:59:01Z

The CI failed because libcxx uses offsetof.
https://github.com/kasuga-fj/llvm-project/blob/b9c767a6a128f57858273eb43d4383e83d0d086b/libcxx/include/__type_traits/datasizeof.h#L50
I investigated but couldn't figure out where to fix it (Should I change the compile time options?). Could someone please help me? It may be unrelated, but I am also concerned that "libc++ test suite in C++03" failed while "libc++ test suite in C++26" succeeded.

tru · 2023-09-05T09:09:10Z

Ping @llvm/pr-subscribers-libcxx

AaronBallman

Hmmm, in some ways I think this is making the diagnostic needlessly chatty, and in other ways the new diagnostic behavior seems reasonable.

On balance, I think the improvements outweigh the regressions, especially for code users are likely to hit. e.g., I suspect there's way more use of offsetof in a static assertion than there are uses of offsetof as an operand to sizeof.

AaronBallman · 2023-09-05T14:18:51Z

clang/test/SemaCXX/offsetof.cpp

@@ -17,7 +17,7 @@ struct Base { int x; };
 struct Derived : Base { int y; };
 int o = __builtin_offsetof(Derived, x); // expected-warning{{offset of on non-POD type}}

-const int o2 = sizeof(__builtin_offsetof(Derived, x));
+const int o2 = sizeof(__builtin_offsetof(Derived, x)); // expected-warning{{offset of on non-POD type 'Derived'}}


This is an example of why I think the existing behavior is correct. The fact that we're asking for the offset of a field on a non-POD type is not harmful because the call is never evaluated.

So you think we should ie, check for !isUnevaluated() - ie diag in constant expressions but not in unevaluated contexts?

I think that might produce slightly better results by silencing this kind of warning.

Fixed. Thanks.

AaronBallman · 2023-09-05T14:20:10Z

clang/test/SemaCXX/ms_struct.cpp

@@ -25,6 +25,7 @@ struct B : public A {

 static_assert(__builtin_offsetof(B, d) == 12,
  "We can't allocate the bitfield into the padding under ms_struct");
+// expected-warning@-2 {{offset of on non-standard-layout type 'B'}}


I think this diagnostic is an improvement over the status quo.

AaronBallman · 2023-09-05T14:20:37Z

clang/test/SemaCXX/offsetof.cpp

-int derived1[__builtin_offsetof(Derived2, x) == 0? 1 : -1];
-int derived2[__builtin_offsetof(Derived2, y)  == 4? 1 : -1];
-int derived3[__builtin_offsetof(Derived2, z)  == 8? 1 : -1];
+int derived1[__builtin_offsetof(Derived2, x) == 0? 1 : -1]; // expected-warning{{offset of on non-POD type 'Derived2'}}


This also seems like an improvement in diagnostic behavior.

kasuga-fj · 2023-09-20T05:08:34Z

@llvm/pr-subscribers-libcxx Could you please check this comment? #65246 (comment)

cor3ntin · 2023-09-21T16:08:52Z

@philnik777 may be able to help

ldionne · 2023-09-21T21:46:28Z

I think when the @llvm/pr-subscribers-libcxx group was pinged 2 weeks ago, most of us didn't have the right GH notifications set up yet and we didn't see it. Looking now.

ldionne · 2023-09-21T21:49:22Z

Note that you should probably rebase your patch onto main and force-push to update the PR. The CI for Clang now runs on Windows from GH PRs as well (it used not to when you submitted the patch, I think).

ldionne · 2023-09-21T22:09:07Z

Ok so after scratching my head a bit, I'm actually not certain of how to proceed with this. I can't find a way to implement __libcpp_datasizeof in libc++ that doesn't involve offsetof, so our only option would be to silence the warning. We could do it like (if we end up doing that, just take that diff and incorporate it to your patch, we can land both at once and the libc++ CI will also trigger):

diff --git a/libcxx/include/__type_traits/datasizeof.h b/libcxx/include/__type_traits/datasizeof.h
index 019099a9cf18..88a702470c3c 100644
--- a/libcxx/include/__type_traits/datasizeof.h
+++ b/libcxx/include/__type_traits/datasizeof.h
@@ -47,7 +47,11 @@ struct __libcpp_datasizeof {
   };
 #endif
 
+  _LIBCPP_DIAGNOSTIC_PUSH
+  _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")
+  _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")
   static const size_t value = offsetof(_FirstPaddingByte<>, __first_padding_byte_);
+  _LIBCPP_DIAGNOSTIC_POP
 };
 
 _LIBCPP_END_NAMESPACE_STD

However, in C++03, using offsetof with a non-POD is undefined behavior (hence the warning, I guess). So we'd technically have UB in our code here, which isn't great. In C++17 doing that is conditionally supported, so I guess we're fine.

So... should we strive to eradicate this from libc++ in the first place, or should we just silence the warning. WDYT?

Also, given that it is conditionally supported in C++17, I assume this warning doesn't trigger at all in C++17 mode, right?

kasuga-fj · 2023-09-22T06:06:45Z

Thank you for your review! Here are some comments and my thoughts.

Note that you should probably rebase your patch onto main and force-push to update the PR

Does it mean that I can rebase and force-push since (it should be avoided in normal case but) this is a special case?

So... should we strive to eradicate this from libc++ in the first place, or should we just silence the warning. WDYT?

I think it's good to only silence the warning and (if necessary) leave some TODO comments about the offsetof usage because it's undesirable for this patch to be larger than it needs to be..

Also, given that it is conditionally supported in C++17, I assume this warning doesn't trigger at all in C++17 mode, right?

I believe the purpose of this warning is to tell you that your program works now, but may not be compiled with another compiler. So the similar warning should appear even in C++17 mode. I chedked and found that _FirstPaddingByte<> is ~~standard layout but non-POD~~ both standard-layout and POD (I checked them with std::is_standard_layout and std::is_pod). Using offsetof with a standard-layout class is supported in C++17, so I guess there is no warning

ldionne · 2023-09-25T13:18:34Z

Thank you for your review! Here are some comments and my thoughts.

Note that you should probably rebase your patch onto main and force-push to update the PR

Does it mean that I can rebase and force-push since (it should be avoided in normal case but) this is a special case?

That's what I usually do. Do we have any Github workflow documentation that mentions we shouldn't force-push to update PRs? If so, I am not aware of it.

So... should we strive to eradicate this from libc++ in the first place, or should we just silence the warning. WDYT?

I think it's good to only silence the warning and (if necessary) leave some TODO comments about the offsetof usage because it's undesirable for this patch to be larger than it needs to be..

My question is about the desired end state of libc++, not whether that end state should be achieved in this specific patch or not.

Also, given that it is conditionally supported in C++17, I assume this warning doesn't trigger at all in C++17 mode, right?

I believe the purpose of this warning is to tell you that your program works now, but may not be compiled with another compiler. So the similar warning should appear even in C++17 mode. I chedked and found that _FirstPaddingByte<> is ~~standard layout but non-POD~~ both standard-layout and POD (I checked them with std::is_standard_layout and std::is_pod). Using offsetof with a standard-layout class is supported in C++17, so I guess there is no warning

What T type did you use to check that _FirstPaddingByte<> was standard layout and POD? Certainly _FirstPaddingByte<> can't be standard layout if the T it holds is not standard layout?

@philnik777 do you have thoughts on this?

kasuga-fj · 2023-09-26T09:33:31Z

Do we have any Github workflow documentation that mentions we shouldn't force-push to update PRs?

According to the LLVM GitHub User Guide, we should generally avoid rebasing and pushing force. But I trust it will be allowed in this case.

My question is about the desired end state of libc++, not whether that end state should be achieved in this specific patch or not.

I misunderstood, sorry. I'm not an expert of libcxx, so I cannot determine what is should be (although it is only what an amateur thinks, at least it seems to be working fine now, so just suppressing warning would not be a big problem, I think).
Incidentally, as you may have already noticed, it may be worth noting that a relatively recent commit introduced __libcpp_datasizeof (c4e9872).

Certainly _FirstPaddingByte<> can't be standard layout if the T it holds is not standard layout?

Yes, you are correct. I was only checking with standard-layout class... I tried passing a non-standard layout class in the type parameter and _FirstPaddingByte<> became non standard-layout. By the way, I checked the CI's failure log and it looks that perhaps there is no test which uses __libcpp_datasizeof with non standard-layout type (I may have missed something). I think it would be better adding such one.

philnik777 · 2023-09-26T09:58:04Z

Assuming that Clang doesn't plan on optimizing anything based on this, I think it should be fine to just silence the warning. We could also add a __datasizeof builtin in a separate patch if we want. That would probably be less error-prone and cheaper than our current solution. FWIW we'll need some version of this when we want to replace __compressed_pair, so a cheaper solution would probably be a good idea.

ldionne · 2023-09-27T13:25:44Z

@kasuga-fj Ok, let's take the diff I posted earlier in the comments and silence the warning in libc++ then. Once you do that, the libc++ test suite shouldn't fail with your patch anymore so your CI should be green. Then libc++ will be out of your way and it's up to the Clang folks whether this is worth doing.

llvmbot · 2023-09-28T09:12:16Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-libcxx

Changes

Fixes #64619

Clang warns diagnostic for non-standard layout types in offsetof only if they are in evaluated context. With this patch, you'll also get diagnostic if you use offsetof on non-standard layout types in any other contexts

Full diff: https://github.com/llvm/llvm-project/pull/65246.diff

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/lib/Sema/SemaExpr.cpp (+4-5)
(modified) clang/test/SemaCXX/class-layout.cpp (+15-15)
(modified) clang/test/SemaCXX/ms_struct.cpp (+3-3)
(modified) clang/test/SemaCXX/offsetof.cpp (+5-4)
(modified) libcxx/include/__type_traits/datasizeof.h (+4)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f314c9c72fa28b7..abc36bac05d788d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -208,6 +208,8 @@ Improvements to Clang's diagnostics
   (`#54678: <https://github.com/llvm/llvm-project/issues/54678>`_).
 - Clang now prints its 'note' diagnostic in cyan instead of black, to be more compatible
   with terminals with dark background colors. This is also more consistent with GCC.
+- Clang now always diagnoses when using non-standard layout types in ``offsetof`` .
+  (`#64619: <https://github.com/llvm/llvm-project/issues/64619>`_)
 
 Bug Fixes in This Version
 -------------------------
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 92496b03ecabe54..fedea932c5e15fe 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -16766,12 +16766,11 @@ ExprResult Sema::BuildBuiltinOffsetOf(SourceLocation BuiltinLoc,
         LangOpts.CPlusPlus11? diag::ext_offsetof_non_standardlayout_type
                             : diag::ext_offsetof_non_pod_type;
 
-      if (!IsSafe && !DidWarnAboutNonPOD &&
-          DiagRuntimeBehavior(BuiltinLoc, nullptr,
-                              PDiag(DiagID)
-                              << SourceRange(Components[0].LocStart, OC.LocEnd)
-                              << CurrentType))
+      if (!IsSafe && !DidWarnAboutNonPOD && !isUnevaluatedContext()) {
+        Diag(BuiltinLoc, DiagID)
+            << SourceRange(Components[0].LocStart, OC.LocEnd) << CurrentType;
         DidWarnAboutNonPOD = true;
+      }
     }
 
     // Look for the field.
diff --git a/clang/test/SemaCXX/class-layout.cpp b/clang/test/SemaCXX/class-layout.cpp
index 9782ff08100b2d6..23b3dbe24249378 100644
--- a/clang/test/SemaCXX/class-layout.cpp
+++ b/clang/test/SemaCXX/class-layout.cpp
@@ -1,18 +1,18 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-apple-darwin    %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
-// RUN: %clang_cc1 -triple x86_64-scei-ps4        %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-sie-ps5         %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
-// RUN: %clang_cc1 -triple powerpc-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
-// RUN: %clang_cc1 -triple powerpc-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
-// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
-// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
-// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-invalid-offsetof -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof
+// RUN: %clang_cc1 -triple x86_64-apple-darwin    %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-scei-ps4        %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-sie-ps5         %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
+// RUN: %clang_cc1 -triple powerpc-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple powerpc-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix7.3.0.0 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof
+// RUN: %clang_cc1 -triple s390x-none-zos %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-invalid-offsetof -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
 
 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
diff --git a/clang/test/SemaCXX/ms_struct.cpp b/clang/test/SemaCXX/ms_struct.cpp
index e6f0a25b38ea80c..995e424d1f87640 100644
--- a/clang/test/SemaCXX/ms_struct.cpp
+++ b/clang/test/SemaCXX/ms_struct.cpp
@@ -25,6 +25,7 @@ struct B : public A {
 
 static_assert(__builtin_offsetof(B, d) == 12,
   "We can't allocate the bitfield into the padding under ms_struct");
+// expected-warning@-2 {{offset of on non-standard-layout type 'B'}}
 
 struct C {
 #ifdef TEST_FOR_ERROR
@@ -38,6 +39,5 @@ struct C {
 
 static_assert(__builtin_offsetof(C, n) == 8,
               "long long field in ms_struct should be 8-byte aligned");
-#if !defined(TEST_FOR_ERROR) && !defined(TEST_FOR_WARNING)
-// expected-no-diagnostics
-#endif
+// expected-warning@-2 {{offset of on non-standard-layout type 'C'}}
+
diff --git a/clang/test/SemaCXX/offsetof.cpp b/clang/test/SemaCXX/offsetof.cpp
index c984657ebe1f0ed..c4e86369f007dcb 100644
--- a/clang/test/SemaCXX/offsetof.cpp
+++ b/clang/test/SemaCXX/offsetof.cpp
@@ -51,9 +51,9 @@ struct Derived2 : public Base1, public Base2 {
   int z; 
 };
 
-int derived1[__builtin_offsetof(Derived2, x) == 0? 1 : -1];
-int derived2[__builtin_offsetof(Derived2, y)  == 4? 1 : -1];
-int derived3[__builtin_offsetof(Derived2, z)  == 8? 1 : -1];
+int derived1[__builtin_offsetof(Derived2, x) == 0? 1 : -1]; // expected-warning{{offset of on non-POD type 'Derived2'}}
+int derived2[__builtin_offsetof(Derived2, y)  == 4? 1 : -1]; // expected-warning{{offset of on non-POD type 'Derived2'}}
+int derived3[__builtin_offsetof(Derived2, z)  == 8? 1 : -1]; // expected-warning{{offset of on non-POD type 'Derived2'}}
 
 // offsetof referring to anonymous struct in base.
 // PR7769
@@ -66,7 +66,8 @@ struct foo {
 struct bar : public foo  {
 };
 
-int anonstruct[__builtin_offsetof(bar, x) == 0 ? 1 : -1];
+int anonstruct[__builtin_offsetof(bar, x) == 0 ? 1 : -1]; // expected-warning{{offset of on non-POD type 'bar'}}
+
 
 struct LtoRCheck {
   int a[10];
diff --git a/libcxx/include/__type_traits/datasizeof.h b/libcxx/include/__type_traits/datasizeof.h
index 019099a9cf183b5..88a702470c3c15a 100644
--- a/libcxx/include/__type_traits/datasizeof.h
+++ b/libcxx/include/__type_traits/datasizeof.h
@@ -47,7 +47,11 @@ struct __libcpp_datasizeof {
   };
 #endif
 
+  _LIBCPP_DIAGNOSTIC_PUSH
+  _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")
+  _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")
   static const size_t value = offsetof(_FirstPaddingByte<>, __first_padding_byte_);
+  _LIBCPP_DIAGNOSTIC_POP
 };
 
 _LIBCPP_END_NAMESPACE_STD

phyBrackets · 2023-09-28T09:54:21Z

libcxx/include/__type_traits/datasizeof.h

@@ -47,7 +47,11 @@ struct __libcpp_datasizeof {
  };
 #endif



Optional : Can you add a brief comment/note that why warnings are suppressed here?
Mb something like :

// Disable warnings related to the use of `offsetof` on non-standard layout types // TODO : Find a way to replace offsetof ?

Done. Thanks

kasuga-fj · 2023-09-29T09:55:21Z

Previous CI failures due to offsetof warnings have been resolved. Thank you for your cooperation! The current CI error (libcxx-ci Benchmark) does not seem to be caused by this patch (other PRs seem to be failing in the same one), so I'll wait until it's fixed.

philnik777 · 2023-09-29T12:15:14Z

libcxx/include/__type_traits/datasizeof.h

+  // TODO : Find a way to replace `offsetof` ?
+  _LIBCPP_DIAGNOSTIC_PUSH
+  _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")
+  _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Winvalid-offsetof")


GCC doesn't warn on this, so we shouldn't suppress anything here.

philnik777 · 2023-09-29T12:16:46Z

libcxx/include/__type_traits/datasizeof.h

@@ -47,7 +47,13 @@ struct __libcpp_datasizeof {
  };
 #endif

+  // Disable warnings related to the use of `offsetof` on non-standard layout or non-POD types
+  // TODO : Find a way to replace `offsetof` ?


I don't think this TODO is actionable from the libc++ side of things currently, so I'd rather not add it. offsetof is the right tool for what we are trying to achieve - even if it's technically UB.

libcxx/include/__type_traits/datasizeof.h

ldionne · 2023-09-29T15:51:31Z

Previous CI failures due to offsetof warnings have been resolved. Thank you for your cooperation! The current CI error (libcxx-ci Benchmark) does not seem to be caused by this patch (other PRs seem to be failing in the same one), so I'll wait until it's fixed.

The CI issues with the Benchmarks should be fixed now, so if you rebase onto main it shouldn't be a problem anymore

github-actions · 2023-10-02T00:57:35Z

✅ With the latest revision this PR passed the C/C++ code formatter.

philnik777

LGTM from the libc++ side of things.

kasuga-fj · 2023-10-02T08:27:13Z

Thank you for your review! (The upstream build process seems to be broken now, so I'll wait until it's fixed)

`offsetof` Fixes llvm#64619 Clang warns diagnostic for non-standard layout types in `offsetof` only if they are in evaluated context. With this patch, you'll also get diagnostic if you use `offsetof` on non-standard layout types in any other contexts

kasuga-fj requested a review from a team as a code owner September 4, 2023 05:58

cor3ntin added clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Sep 4, 2023

tru reviewed Sep 4, 2023

View reviewed changes

clang/test/SemaCXX/ms_struct.cpp Outdated Show resolved Hide resolved

kasuga-fj changed the title ~~[Clang] Fix missing diagnostic for non-standard layout type in~~ [Clang] Fix missing diagnostic for non-standard layout type in offsetof Sep 4, 2023

AaronBallman approved these changes Sep 5, 2023

View reviewed changes

cor3ntin approved these changes Sep 6, 2023

View reviewed changes

ldionne self-requested a review September 21, 2023 22:09

kasuga-fj force-pushed the bugfix/issue-64619-enhance-diagnostic-for-offsetof branch from 7faabb2 to 10d759e Compare September 28, 2023 09:11

kasuga-fj requested a review from a team as a code owner September 28, 2023 09:11

llvmbot added clang Clang issues not falling into any other category libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. labels Sep 28, 2023

phyBrackets reviewed Sep 28, 2023

View reviewed changes

philnik777 reviewed Sep 29, 2023

View reviewed changes

kasuga-fj force-pushed the bugfix/issue-64619-enhance-diagnostic-for-offsetof branch from 1acbf7d to b9e201a Compare October 2, 2023 00:54

philnik777 approved these changes Oct 2, 2023

View reviewed changes

kasuga-fj force-pushed the bugfix/issue-64619-enhance-diagnostic-for-offsetof branch 3 times, most recently from aa41086 to 5721932 Compare October 5, 2023 08:29

kasuga-fj force-pushed the bugfix/issue-64619-enhance-diagnostic-for-offsetof branch from 5721932 to 01ca658 Compare October 6, 2023 01:05

kasuga-fj merged commit 5fa5ffe into llvm:main Oct 10, 2023

kasuga-fj deleted the bugfix/issue-64619-enhance-diagnostic-for-offsetof branch October 10, 2023 06:31

stepthomas mentioned this pull request Oct 10, 2023

AMDGPU stepthomas atomic csub no rtn forms ver2 stepthomas/llvm-project#1

Closed

[Clang] Fix missing diagnostic for non-standard layout type in offsetof #65246

[Clang] Fix missing diagnostic for non-standard layout type in offsetof #65246

Conversation

kasuga-fj commented Sep 4, 2023 • edited Loading

cor3ntin commented Sep 4, 2023

cor3ntin commented Sep 4, 2023

kasuga-fj commented Sep 4, 2023

tbaederr commented Sep 4, 2023

cor3ntin commented Sep 4, 2023

metaflow commented Sep 4, 2023

tbaederr commented Sep 4, 2023

kasuga-fj commented Sep 5, 2023

hazohelet commented Sep 5, 2023

kasuga-fj commented Sep 5, 2023

hazohelet commented Sep 5, 2023

metaflow commented Sep 5, 2023

kasuga-fj commented Sep 5, 2023

tru commented Sep 5, 2023

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasuga-fj commented Sep 20, 2023

cor3ntin commented Sep 21, 2023

ldionne commented Sep 21, 2023

ldionne commented Sep 21, 2023

ldionne commented Sep 21, 2023

kasuga-fj commented Sep 22, 2023 • edited Loading

ldionne commented Sep 25, 2023

kasuga-fj commented Sep 26, 2023

philnik777 commented Sep 26, 2023

ldionne commented Sep 27, 2023

llvmbot commented Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasuga-fj commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldionne commented Sep 29, 2023

github-actions bot commented Oct 2, 2023 • edited Loading

philnik777 left a comment

Choose a reason for hiding this comment

kasuga-fj commented Oct 2, 2023 • edited Loading

[Clang] Fix missing diagnostic for non-standard layout type in `offsetof` #65246

[Clang] Fix missing diagnostic for non-standard layout type in `offsetof` #65246

kasuga-fj commented Sep 4, 2023 •

edited

Loading

kasuga-fj commented Sep 22, 2023 •

edited

Loading

llvmbot commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Oct 2, 2023 •

edited

Loading

kasuga-fj commented Oct 2, 2023 •

edited

Loading