Missing diagnostic for non-standard layout types in offsetof

[Endilll]

Consider the following example (https://godbolt.org/z/o77v1nad9):

struct X1 { int a; };
struct X2 { int b; };
struct Y : X1, X2 {};

static_assert(__builtin_offsetof(Y, b) == 4);
static_assert(offsetof(Y, b) == 4);

static_assert(!std::is_standard_layout_v<Y>, "");

Only GCC warns that non-standard layout type is passed to offsetof, which is conditionally-supported.

<source>:8:34: warning: 'offsetof' within non-standard-layout type 'Y' is conditionally-supported [-Winvalid-offsetof]
    8 | static_assert(__builtin_offsetof(Y, b) == 4);
      |               ~~~~~~~~~~~~~~~~~~~^~~~~
In file included from /opt/compiler-explorer/gcc-trunk-20230811/include/c++/14.0.0/cstddef:50,
                 from <source>:1:
<source>:9:24: warning: 'offsetof' within non-standard-layout type 'Y' is conditionally-supported [-Winvalid-offsetof]
    9 | static_assert(offsetof(Y, b) == 4);
      |                        ^

GCC issues it by default, whereas Clang, MSVC, and EDG doesn't even in pedantic modes (or /W4).

I'm not sure what the solution should be, but I feel like there is a problem to solve.

[Endilll]

CC @AaronBallman
This might be a good first issue, depending on how we're going to resolve this.

[shafik]

Interesting we do have ext_offsetof_non_standardlayout_type and we do use the diagnostic here

llvm-project/clang/lib/Sema/SemaExpr.cpp

Lines 16745 to 16753 in e6f18b7

	// C++11 [support.types]p4:
	// If type is not a standard-layout class (Clause 9), the results are
	// undefined.
	if (CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
	bool IsSafe = LangOpts.CPlusPlus11? CRD->isStandardLayout() : CRD->isPOD();
	unsigned DiagID =
	LangOpts.CPlusPlus11? diag::ext_offsetof_non_standardlayout_type
	: diag::ext_offsetof_non_pod_type;

I haven't debugged it to see why we don't catch this case.

[cor3ntin]

We only warn in evaluated context. I don't know if that make sense https://godbolt.org/z/MGTWofPxY
Maybe we don't want to use DiagRuntimeBehavior here

[AaronBallman]

We only warn in evaluated context. I don't know if that make sense https://godbolt.org/z/MGTWofPxY Maybe we don't want to use DiagRuntimeBehavior here

It's been this way since it was added in 882211c in 2010. That said, I'm not certain why this should be issued only when evaluated at runtime. My guess is that it's a runtime diagnostic because the offset is expected to be used at runtime to offset into some memory buffer; so if you never reach that code, the fact that it's conditionally supported doesn't matter. But I'm not certain if that's an accurate guess -- I could not find a code review to see if this was explicitly discussed.

[cor3ntin]

I can see that reasoning be problematic, for example:

void f() {
   constexpr auto idx = offsetof(nonstandardlayout, foo); 
   loose_a_limb(idx);
}

[AaronBallman]

Yeah, basically 2010 predates thinking about constexpr too much, so this might have made more sense then and less sense now.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Assign the issue to you.
2. Fix the issue locally.
3. Run the test suite locally.
   3.1) Remember that the subdirectories under test/ create fine-grained testing targets, so you can
   e.g. use make check-clang-ast to only run Clang's AST tests.
4. Create a git commit
5. Run git clang-format HEAD~1 to format your changes.
6. Submit the patch to Phabricator.
   6.1) Detailed instructions can be found here

For more instructions on how to submit a patch to LLVM, see our documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment on this Github issue.

@llvm/issue-subscribers-good-first-issue

[kasuga-fj]

Hi, I am new to LLVM and would like to try to see if this issue is still open. Could someone please set up an assignee for me? (I'll send a patch after the pull request migration is done)

[kawashima-fj]

Hi, I am new to LLVM and would like to try to see if this issue is still open.

Great.

Could someone please set up an assignee for me?

Done.

[kasuga-fj]

Thank you