[C2x] reject type definitions in offsetof

[inclyc]

Summary

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm made very
clear that it is an UB having type definitions with in offsetof. After
this patch clang will reject any type definitions in __builtin_offsetof.

Fixes https://github.com/llvm/llvm-project/issues/57065

local/offsetof.c:10:38: error: 'struct S' cannot be defined in '__builtin_offsetof'
    return __builtin_offsetof(struct S{ int a, b;}, a);
                                     ^

[inclyc]

Apply clang-format

[inclyc]

Add release notes

[inclyc]

Use double backquotes

[aaron.ballman]

Thank you for working on this! This approach can work (though I would recommend using an RAII object akin to InMessageExpressionRAIIObject), but have you explored making a new DeclSpecContext and modifying isDefiningTypeSpecifierContext()? I think that would likely be a cleaner approach.

[inclyc]

Use RAII object to maintain the Parser state

have you explored making a new DeclSpecContext and modifying isDefiningTypeSpecifierContext()? I think that would likely be a cleaner approach.

Emm, I've tried passing a DeclaratorContext into ParseTypeName()

SourceLocation TypeLoc = Tok.getLocation();
InBuiltInOffsetOfBaseRAIIObject InOffsetof(*this, true);
TypeResult Ty = ParseTypeName(nullptr, /*Context=???*/);

But defining a new DeclaratorContext I have to complete a bunch of case
statements.

// Parser.h
static bool isTypeSpecifier(DeclSpecContext DSC);
static AllowDefiningTypeSpec isDefiningTypeSpecifierContext(DeclSpecContext DSC, bool IsCPlusPlus);
static bool isOpaqueEnumDeclarationContext(DeclSpecContext DSC);
/* ... */

And I think it is somehow strange to determine these properties within
__builtin_offsetof()? I'm not sure if it is really appropriate to define a
special context for a built-in function. This place should only need to
forbidden definitions, right?

[aaron.ballman]

In D133574#3781907, @inclyc wrote:

Use RAII object to maintain the Parser state

have you explored making a new DeclSpecContext and modifying isDefiningTypeSpecifierContext()? I think that would likely be a cleaner approach.

Emm, I've tried passing a DeclaratorContext into ParseTypeName()

SourceLocation TypeLoc = Tok.getLocation();
InBuiltInOffsetOfBaseRAIIObject InOffsetof(*this, true);
TypeResult Ty = ParseTypeName(nullptr, /*Context=???*/);

But defining a new DeclaratorContext I have to complete a bunch of case
statements.

// Parser.h
static bool isTypeSpecifier(DeclSpecContext DSC);
static AllowDefiningTypeSpec isDefiningTypeSpecifierContext(DeclSpecContext DSC, bool IsCPlusPlus);
static bool isOpaqueEnumDeclarationContext(DeclSpecContext DSC);
/* ... */

And I think it is somehow strange to determine these properties within
__builtin_offsetof()? I'm not sure if it is really appropriate to define a
special context for a built-in function. This place should only need to
forbidden definitions, right?

The thing is: it really is a declaration context; one in which type declarations are not allowed. So yes, you do have to fill out a bunch of fully covered switches in places, but I still think that's the correct way to model this instead of introducing a one-off RAII object.

[inclyc]

Use declaration context

[shafik]

clang/include/clang/Parse/Parser.h

2298

Why true for this case? What does this allow that we want? Do we test it?

clang/lib/Parse/ParseExpr.cpp

2594

[inclyc]

Address comments

[inclyc]

git-clang-format

[aaron.ballman]

clang/include/clang/Parse/Parser.h

2328

Is this correct? I don't think we can deduce the type from offsetof through CTAD, can we?

https://godbolt.org/z/Kab6ahYe7

(This might be a good test case to add assuming we don't already have that coverage.)

clang/include/clang/Sema/DeclSpec.h

2069 ↗

(On Diff #459634)

I don't think we can omit the identifier in this context -- this function is used to see whether you can do something like:

template <int>
void func(int) {
  try {
  } catch (int) {
  }
}

and offsetof seems quite different from that.

clang/test/Sema/offsetof.c

79 ↗

(On Diff #459634)

I think this is defensible. The wording in the standard is "If the specified type defines a new type or if the specified member is a bit-field, the behavior is undefined." and the specified type in this case is struct A; that struct A happens to also define struct B is immaterial.

However, the intent behind the change to the rule is to support older implementations of offsetof to protect them from having to deal with a case like: offsetof(struct S { int a, b }, b); where offsetof is a macro and thus the comma between a and b is treated as a separator. So there's a part of me that wonders if we want to also support diagnosing this case. But then we'd have to look at the declarator context more recursively to see whether any of the contexts on the stack are an offsetof context and that might be tricky.

Thoughts?

[inclyc]

clang/include/clang/Parse/Parser.h

2328

Emm, these checks just return as the same as "type_specifier". Because that's what we passed into "ParsingTypename" before.

clang/test/Sema/offsetof.c

79 ↗

(On Diff #459634)

FWIW, gcc seems just rejects all definitions in this context. (Perhaps during Parsing the statements). If we add a bool state to the Parser (just using RAII object as before) struct B will trigger diagnostic error because the state "ParsingOffsetof" is passed into inner declaration.

[aaron.ballman]

clang/include/clang/Parse/Parser.h

2328

Hmmm, that's certainly reasonable. Can you add the test case and make sure the behavior doesn't change?

clang/test/Sema/offsetof.c

79 ↗

(On Diff #459634)

GCC accepts currently: https://godbolt.org/z/oEvzjW6Ee but you're correct regarding switching back to an RAII object being an easier way to address the nested declarations.

Let me think on this situation a bit....

[inclyc]

clang/test/Sema/offsetof.c

79 ↗

(On Diff #459634)

GCC accepts currently

C++: https://godbolt.org/z/fon8e7dzf

79 ↗

(On Diff #459634)

<source>: In function 'int main()':
<source>:3:3: error: types may not be defined within '__builtin_offsetof'
    3 |   {
      |   ^
<source>:6:5: error: types may not be defined within '__builtin_offsetof'
    6 |     {
      |     ^
Compiler returned: 1

[aaron.ballman]

clang/test/Sema/offsetof.c

79 ↗

(On Diff #459634)

C++ is a different language in this case though. In C, you can generally define types anywhere you can spell a type, and in C++ you cannot. e.g., void func(struct S { int x, y; } s); is valid in C and invalid in C++.

[inclyc]

clang/test/Sema/offsetof.c

79 ↗

(On Diff #459634)

GCC explicitly reject type definitions since GCC 8 (in C++ mode). I guess the logic here in GCC may also add a boolean state parameter to parser.

Interestingly, in C++ we treat the first parameter of __builtin_offsetof as a type specifier, rejecting the definition of struct A, but not rejecting nested definition struct B

https://godbolt.org/z/PqWjzqqYn

Emm, so, how about switch back to RAIIObject to support diagnosing nested definitions?

[aaron.ballman]

clang/test/Sema/offsetof.c

79 ↗

(On Diff #459634)

Interestingly, in C++ we treat the first parameter of __builtin_offsetof as a type specifier, rejecting the definition of struct A, but not rejecting nested definition struct B

Yeah, that is interesting!

Emm, so, how about switch back to RAIIObject to support diagnosing nested definitions?

I'm getting more comfortable with that approach. Please be sure to add C++ tests to make sure we diagnose in C and C++ the same way in terms of nested structures. Thank you for exploring the approaches with me!

[inclyc]

Switch back to RAIIObject.

Currently clang could not generate diagnostic messages for nested definitions
in C++. I believe using RAIIObject here is logically correct, but in C++ mode,
ActOnTag returns nullptr, and the comment says

We can't recover well from the cases where we make the type anonymous

/* SemaDecl.cpp Sema::ActOnTag Ln 17189 */
// In C++, don't return an invalid declaration. We can't recover well from
// the cases where we make the type anonymous.
if (Invalid && getLangOpts().CPlusPlus) {
  if (New->isBeingDefined())
    if (auto RD = dyn_cast<RecordDecl>(New))
      RD->completeDefinition();
  return nullptr;
} else if (SkipBody && SkipBody->ShouldSkip) {
  return SkipBody->Previous;
} else {
  return New;
}

I believe the state "ParsingBuiltinOffsetof" is correctly passed into
ParseCXXMemberSpecification (parsing nested definition), and if clang
recovers invalid declaration in the furture, nested definitions will be caught.

if (TUK == Sema::TUK_Definition) {
  assert(Tok.is(tok::l_brace) ||
         (getLangOpts().CPlusPlus && Tok.is(tok::colon)) ||
         isClassCompatibleKeyword());
  if (SkipBody.ShouldSkip)
    SkipCXXMemberSpecification(StartLoc, AttrFixitLoc, TagType,
                               TagOrTempResult.get());
  else if (getLangOpts().CPlusPlus)
    ParseCXXMemberSpecification(StartLoc, AttrFixitLoc, attrs, TagType,
                                TagOrTempResult.get());  // <----  nullptr, nested declaration skipped
  else {
    Decl *D =
        SkipBody.CheckSameAsPrevious ? SkipBody.New : TagOrTempResult.get();
    // Parse the definition body.
    ParseStructUnionBody(StartLoc, TagType, cast<RecordDecl>(D));
    if (SkipBody.CheckSameAsPrevious &&
        !Actions.ActOnDuplicateDefinition(TagOrTempResult.get(), SkipBody)) {
      DS.SetTypeSpecError();
      return;
    }
  }
}

[inclyc]

rebase

[aaron.ballman]

clang/docs/ReleaseNotes.rst

607–608

You should move this one down to the C2x Feature Support section instead, since it's a C2x paper.

clang/include/clang/Basic/DiagnosticSemaKinds.td

1654

We might want this change, we might not -- can you test the diagnostic behavior when using #include <stddef.h>? Does it print __builtin_offsetof in the following example?

#include <stddef.h>

int main() {
  return offsetof(struct S { int a; }, a);
}

when executed with clang -fsyntax-only -ffreestanding -std=c2x test.c

If it prints the builtin name, I think we'll want to look at the builtin token to see if it was expanded from a macro named offsetof to improve the diagnostic quality.

clang/lib/Sema/SemaDecl.cpp

17369

You should be able to pass in the TagDecl directly because the diagnostics engine knows how to print a NamedDecl.

clang/test/Sema/offsetof.c

73–75 ↗

(On Diff #461822)

Can you move this into a test named clang/test/C/C2x/n2350.c which looks like the other tests in that directory? That's the testing directory we're putting some basic validation tests for C papers we've implemented so that we can better track what features we claim to support.

clang/test/SemaCXX/offsetof.cpp

93

I'd like a FIXME comment here about wanting to diagnose this someday.

[inclyc]

Emm, is it necessary to add a LangOpts check so that this change only applies to c2x? If clang was invoked without -std=c2x, should we just accept offsetof with definitions?

clang/include/clang/Basic/DiagnosticSemaKinds.td

1654

local/offsetofcc.c:4:26: error: 'struct S' cannot be defined in '__builtin_offsetof'
  return offsetof(struct S { int a; }, a);
                         ^
1 error generated.

If it prints the builtin name, I think we'll want to look at the builtin token to see if it was expanded from a macro named offsetof to improve the diagnostic quality.

OK

[aaron.ballman]

In D133574#3811496, @inclyc wrote:

Emm, is it necessary to add a LangOpts check so that this change only applies to c2x? If clang was invoked without -std=c2x, should we just accept offsetof with definitions?

No, I think we should treat this as a defect report and have the behavior be the same in all versions of C.

clang/include/clang/Basic/DiagnosticSemaKinds.td

1654

We have similar code for this here:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaChecking.cpp#L13438

[inclyc]

clang/include/clang/Basic/DiagnosticSemaKinds.td

1654

We have similar code for this here:

Wow! Thank you so much for this. I'm searching for how to do this in a LOT of doxygen generated pages (

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

1654

No problem! I realized "this could be tricky, surely we've done this before" and went looking. :-D

[shafik]

LGTM but I will Aaron give the final approval.

[inclyc]

Address comments.

Clang will now consider __builtin_offsetof #defined from "offsetof" to improve
diagnostic message.

For example:

#define offsetof(t, d) __builtin_offsetof(t, d)

int main() {
  return offsetof(struct S { int a; }, a);
}

local/offsetof.c:4:26: error: 'S' cannot be defined in 'offsetof'
  return offsetof(struct S { int a; }, a);
                         ^
1 error generated.

Emm, the "expected-error" of struct B within a macro seems have to be annotated
at the same line as"struct A".

int macro(void) {
  return offsetof(struct A // expected-error{{'A' cannot be defined in 'offsetof'}}
                           // expected-error@-1{{'B' cannot be defined in 'offsetof'}}     <---- Have to write this here, but I believe the line number is correct
  { 
    int a;
    struct B // FIXME: verifier seems to think the error is emitted by the macro
             // In fact the location of the error is "B" on the line above
    {
      int c;
      int d;
    } x;
  }, a);
}

clang/test/C/C2x/n2350.c:11:36: error: 'A' cannot be defined in '__builtin_offsetof'
  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
                                   ^
clang/test/C/C2x/n2350.c:14:12: error: 'B' cannot be defined in '__builtin_offsetof'
    struct B // expected-error{{'B' cannot be defined in '__builtin_offsetof'}} 
           ^
clang/test/C/C2x/n2350.c:26:26: error: 'A' cannot be defined in 'offsetof'
  return offsetof(struct A // expected-error{{'A' cannot be defined in 'offsetof'}}
                         ^
clang/test/C/C2x/n2350.c:30:12: error: 'B' cannot be defined in 'offsetof' <-- note that this line number is "30" (not "26")
    struct B // FIXME: verifier seems to think the error is emitted by the macro
           ^
4 errors generated.

[inclyc]

Hi @aaron.ballman, I've noticed in the linux kernel, type alignment was implemented by a tricky way using offsetof.

#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)

Does this always has the same semantic with C11 _Alignof? If this is not true, I think unfortunately we may emit a switchable warning instead of an error to preserve semantics here.

[aaron.ballman]

In D133574#3815105, @inclyc wrote:

Hi @aaron.ballman, I've noticed in the linux kernel, type alignment was implemented by a tricky way using offsetof.

#define TYPE_ALIGN(TYPE) offsetof(struct { char x; TYPE test; }, test)

Does this always has the same semantic with C11 _Alignof? If this is not true, I think unfortunately we may emit a switchable warning instead of an error to preserve semantics here.

Technically there's no requirement that they return the same value (the structure could insert arbitrary padding, including no padding), so it's theoretically possible they return different values. But I can't think of a situation in which you'd get a different answer from TYPE_ALIGN as you would get from _Alignof.

@nickdesaulniers any thoughts here since this can impact the Linux kernel?

clang/include/clang/Parse/Parser.h

252–257

clang/include/clang/Sema/Sema.h

3325

Instead of passing two bools, why not pass Parser::OffsetOfStateKind directly?

[inclyc]

clang/include/clang/Sema/Sema.h

3325

Parser::OffsetOfStateKind is not visible to class Sema?

[inclyc]

related kernel patch https://lkml.org/lkml/2022/9/26/1484

[aaron.ballman]

clang/include/clang/Sema/Sema.h

3325

Sure, but it's a new enum, so we can put it into Sema if we want.

[inclyc]

Address comments from @aaron.ballman

Move OffsetOfKind to Sema and pass it to Sema:ActOnTag directly.

[inclyc]

Fix comment nits

[aaron.ballman]

LGTM, thank you! Please don't land until you have some indication from the kernel folks that this won't be super disruptive for them. CC @nickdesaulniers and @nathanchance for awareness.

[nathanchance]

In D133574#3815951, @aaron.ballman wrote:

LGTM, thank you! Please don't land until you have some indication from the kernel folks that this won't be super disruptive for them. CC @nickdesaulniers and @nathanchance for awareness.

Thank you a lot for the heads up! I see an in-flight change around this from YingChi on the mailing list:

https://lore.kernel.org/[e-mail]/

It would be good to have that change accepted into a maintainer's tree and on its way to mainline (Linus's tree) and by extension, the stable releases before merging this into main, as we and several other groups are actively testing all Linux branches with LLVM main to catch other regressions and this error would require patching of our CI, which we can definitely do but prefer to avoid because of the maintenance overhead (we try to carry zero patches at all times).

I am still running my set of builds against the kernel with this LLVM change and the aforementioned Linux change but I do see another error:

drivers/media/platform/nvidia/tegra-vde/v4l2.c:816:49: error: '' cannot be defined in 'offsetof'
        ctx = kzalloc(offsetof(struct tegra_ctx, ctrls[ARRAY_SIZE(ctrl_cfgs)]),
                                                       ^
include/linux/kernel.h:55:59: note: expanded from macro 'ARRAY_SIZE'
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
                                                          ^
include/linux/compiler.h:240:28: note: expanded from macro '__must_be_array'
#define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
                                ^
include/linux/build_bug.h:16:44: note: expanded from macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                           ^
1 error generated.

I will report back with any additional problems found.

[nickdesaulniers]

In D133574#3816066, @nathanchance wrote:

In D133574#3815951, @aaron.ballman wrote:

LGTM, thank you! Please don't land until you have some indication from the kernel folks that this won't be super disruptive for them. CC @nickdesaulniers and @nathanchance for awareness.

Thank you a lot for the heads up! I see an in-flight change around this from YingChi on the mailing list:

https://lore.kernel.org/[e-mail]/

It would be good to have that change accepted into a maintainer's tree and on its way to mainline (Linus's tree) and by extension, the stable releases before merging this into main, as we and several other groups are actively testing all Linux branches with LLVM main to catch other regressions and this error would require patching of our CI, which we can definitely do but prefer to avoid because of the maintenance overhead (we try to carry zero patches at all times).

I am still running my set of builds against the kernel with this LLVM change and the aforementioned Linux change but I do see another error:

drivers/media/platform/nvidia/tegra-vde/v4l2.c:816:49: error: '' cannot be defined in 'offsetof'

^ perhaps we can say something about an anonymous struct rather than print an empty name?

Thanks for the consideration and the kernel patch. Patching the kernel and clang is exceptional, going "above and beyond." Unless we are in a rush to land this, having a little more time for feedback from the kernel thread linked above I think would be good.

In particular, I was surprised by the distinction between __alignof__ and _Alignof semantically.

[aaron.ballman]

In D133574#3816066, @nathanchance wrote:

In D133574#3815951, @aaron.ballman wrote:

LGTM, thank you! Please don't land until you have some indication from the kernel folks that this won't be super disruptive for them. CC @nickdesaulniers and @nathanchance for awareness.

Thank you a lot for the heads up! I see an in-flight change around this from YingChi on the mailing list:

https://lore.kernel.org/[e-mail]/

It would be good to have that change accepted into a maintainer's tree and on its way to mainline (Linus's tree) and by extension, the stable releases before merging this into main, as we and several other groups are actively testing all Linux branches with LLVM main to catch other regressions and this error would require patching of our CI, which we can definitely do but prefer to avoid because of the maintenance overhead (we try to carry zero patches at all times).

I don't think there's a rush to land this immediately, so we can definitely wait for you to give the all-clear.

I am still running my set of builds against the kernel with this LLVM change and the aforementioned Linux change but I do see another error:

drivers/media/platform/nvidia/tegra-vde/v4l2.c:816:49: error: '' cannot be defined in 'offsetof'
        ctx = kzalloc(offsetof(struct tegra_ctx, ctrls[ARRAY_SIZE(ctrl_cfgs)]),
                                                       ^
include/linux/kernel.h:55:59: note: expanded from macro 'ARRAY_SIZE'
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
                                                          ^
include/linux/compiler.h:240:28: note: expanded from macro '__must_be_array'
#define __must_be_array(a)      BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
                                ^
include/linux/build_bug.h:16:44: note: expanded from macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                           ^
1 error generated.

I will report back with any additional problems found.

Oh wow, that's a fun one for a few reasons. 1) Oops, we print '' instead of mention that it's an anonymous struct, 2) that structure is actually declared in the scope surrounding the sizeof so it is technically being defined within offsetof, but 3) The standard requirement is "If the specified type defines a new type or if the specified member is a bit-field, the behavior is undefined." where "type" is the first parameter of the offsetof macro and this example does not define a new type there.

So I *think* this situation is actually a false positive.

[inclyc]

This revision fixes:

anonymous struct

You should be able to pass in the TagDecl directly because the diagnostics
engine knows how to print a NamedDecl.

I've switch back to using Context.getTagDeclType(New) because this can print
anonymous pretty.

clang/test/C/C2x/n2350.c:23:29: error: 'struct (unnamed at clang/test/C/C2x/n2350.c:23:29)' cannot be defined in '__builtin_offsetof'
  return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed at {{.*}})' cannot be defined in '__builtin_offsetof'}}

definitions within the second parameter

I've add a new test case for this. This bug is caused by the RAIIObject having
incorrect scope.

[aaron.ballman]

In D133574#3816688, @inclyc wrote:

This revision fixes:

anonymous struct

You should be able to pass in the TagDecl directly because the diagnostics
engine knows how to print a NamedDecl.

I've switch back to using Context.getTagDeclType(New) because this can print
anonymous pretty.

clang/test/C/C2x/n2350.c:23:29: error: 'struct (unnamed at clang/test/C/C2x/n2350.c:23:29)' cannot be defined in '__builtin_offsetof'
  return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed at {{.*}})' cannot be defined in '__builtin_offsetof'}}

Thanks! I was very surprised we didn't print the anonymous decl properly from the decl itself, so it looks like we have the same bug in other places as well: https://godbolt.org/z/16vP3voTW

definitions within the second parameter

I've add a new test case for this. This bug is caused by the RAIIObject having
incorrect scope.

Thanks!

[nathanchance]

Diff 463063 looks good against the Linux kernel with https://lore.kernel.org/[e-mail]/ applied. I see no additional errors/warnings and the drivers/media/platform/nvidia/tegra-vde/v4l2.c did disappear. Once that patch has been fully chased and accepted, this change should be able to be merged without any problems. Thank you again for taking a look at the kernel ahead of time, it is very much appreciated!

[inclyc]

rebase

[inclyc]

Hi @aaron.ballman! seems we are failing tests on recent change 2fc5a3410087c209567c7e4a2c457b6896c127a3

/* The DR asked a question about whether defining a new type within offsetof
 * is allowed. C2x N2350 made this explicitly undefined behavior, but Clang
 * has always supported defining a type in this location, and GCC also
 * supports it.
 */
 (void)__builtin_offsetof(struct S { int a; }, a);

Do you think this patch should still be merged into mainline? Or do we turn this error into a warning?

[inclyc]

ping :)

[inclyc]

ping :)

[aaron.ballman]

In D133574#4047677, @inclyc wrote:

ping :)

Oh! I missed your pings because this was already marked as accepted, I'm sorry for the delay in answering your question.

In D133574#3994050, @inclyc wrote:

Hi @aaron.ballman! seems we are failing tests on recent change 2fc5a3410087c209567c7e4a2c457b6896c127a3

/* The DR asked a question about whether defining a new type within offsetof
 * is allowed. C2x N2350 made this explicitly undefined behavior, but Clang
 * has always supported defining a type in this location, and GCC also
 * supports it.
 */
 (void)__builtin_offsetof(struct S { int a; }, a);

Do you think this patch should still be merged into mainline? Or do we turn this error into a warning?

We don't currently document __builtin_offsetof and GCC's documentation (https://gcc.gnu.org/onlinedocs/gcc/Offsetof.html) does not explicitly claim they support this behavior, so I'm fine with diagnosing this UB when we previously didn't. If we find this breaks significant code in practice, maybe we'll come to another decision, but for now I'd say you should update that test to expect the error (and update the comment as well).

[bkramer]

GitHub finds around 1.9k instances of this pattern to compute alignof. There's a lot of duplicates and #ifdefs so the real number is going to be smaller, but it seems to be quite common. The annoying part is that there is no _Alignof in C99 so for many of those projects there is no easy fix.

Can this be demoted to a warning?

https://github.com/search?type=code&q=%2Foffsetof%5Cs*%5C%28%5Cs*struct%5Cs*%5C%7B%2F+lang%3Ac

[aaron.ballman]

In D133574#4051782, @bkramer wrote:

GitHub finds around 1.9k instances of this pattern to compute alignof. There's a lot of duplicates and #ifdefs so the real number is going to be smaller, but it seems to be quite common. The annoying part is that there is no _Alignof in C99 so for many of those projects there is no easy fix.

Can this be demoted to a warning?

https://github.com/search?type=code&q=%2Foffsetof%5Cs*%5C%28%5Cs*struct%5Cs*%5C%7B%2F+lang%3Ac

Adding clang-vendors/others with opinions on diagnostic severity because this could be disruptive.

1. Yes, we can.
2. Blarg. I have sympathy because the standard was unclear on this point for so long. I don't have as much sympathy for "there's no easy fix" -- the code is not portable as-is (it's UB), and Clang supports _Alignof in C99 and earlier, so for Clang there is an easy fix.
3. Instead, should we downgrade to a warning-defaults-to-error instead of just a warning? That gives these folks a path forward while still making it clear to users "don't do this." But if we think we won't reasonably be able to turn this into an error, I think a warning is better.
4. Alternatively, should we document that we support this as an extension and only give an extension warning for it?

Regardless, we need to either make a decision shortly or we should revert these changes until after the Clang 16 branch and re-land after that to give ourselves more time to make a decision. If we don't have an obvious "this is the right approach" answer and an implementation by Jan 19 (next Thursday), I think we should revert to give ourselves breathing room.

[aaron.ballman]

Marking this as needing changes so it doesn't look accepted to the new reviewers.

[tlively]

FWIW I had to patch the musl used by Emscripten to work around this: https://github.com/emscripten-core/emscripten/pull/18510

The fix was simple and I don't have a strong opinion about the right way forward, but this change was slightly disruptive and I imagine other projects will have to work around it as well.

[philnik]

In D133574#4051899, @aaron.ballman wrote:

In D133574#4051782, @bkramer wrote:

GitHub finds around 1.9k instances of this pattern to compute alignof. There's a lot of duplicates and #ifdefs so the real number is going to be smaller, but it seems to be quite common. The annoying part is that there is no _Alignof in C99 so for many of those projects there is no easy fix.

Can this be demoted to a warning?

https://github.com/search?type=code&q=%2Foffsetof%5Cs*%5C%28%5Cs*struct%5Cs*%5C%7B%2F+lang%3Ac

Adding clang-vendors/others with opinions on diagnostic severity because this could be disruptive.

1. Yes, we can.
2. Blarg. I have sympathy because the standard was unclear on this point for so long. I don't have as much sympathy for "there's no easy fix" -- the code is not portable as-is (it's UB), and Clang supports _Alignof in C99 and earlier, so for Clang there is an easy fix.
3. Instead, should we downgrade to a warning-defaults-to-error instead of just a warning? That gives these folks a path forward while still making it clear to users "don't do this." But if we think we won't reasonably be able to turn this into an error, I think a warning is better.
4. Alternatively, should we document that we support this as an extension and only give an extension warning for it?

Regardless, we need to either make a decision shortly or we should revert these changes until after the Clang 16 branch and re-land after that to give ourselves more time to make a decision. If we don't have an obvious "this is the right approach" answer and an implementation by Jan 19 (next Thursday), I think we should revert to give ourselves breathing room.

I'm not a vendor, but another option would be to make it a warning-defaults-to-error and indicate that it will be promoted to an error in a few (2?) releases. That makes it less painful, but still make it an error eventually. I don't know if clang has done anything like this before though.

[efriedma]

Given the extension appears to have real-world usage, I don't see any reason to break that usage. All known implementations parse this the way the user expects, there isn't any chance the user meant to write something else, and there isn't any chance it will mean something else in a future version of the standard.

[aaron.ballman]

In D133574#4052293, @efriedma wrote:

Given the extension appears to have real-world usage, I don't see any reason to break that usage. All known implementations parse this the way the user expects, there isn't any chance the user meant to write something else, and there isn't any chance it will mean something else in a future version of the standard.

I've reached out privately to some folks in GCC to find out whether they plan to define this behavior in their builtin or not. One response I've gotten so far is that they expect a type to be allowed to be defined in C but not in C++ (so offsetof has the usual expected behavior in each language). Because of the existing code and because of GCC, I'm leaning pretty strongly towards defining the behavior for C and issuing an extension warning. If anyone has a strong opinion to the contrary, please speak up!

[mstorsjo]

+1 on this being disruptive. Also, not only GCC, but also MSVC seem to handle the existing constructs in the wild. I ran into this at https://github.com/FFmpeg/FFmpeg/blob/n5.1.2/libavutil/video_enc_params.c#L30-L34 (where it probably would be trivial to fix, but leaving current releases broken with newer clang) and at https://github.com/wine-mirror/wine/blob/wine-8.0-rc4/include/winnt.h#L399-L405 (where it works with clang in the usual gnuc mode, but falls back on the UB construct with clang in msvc mode).

[aaron.ballman]

I'll take care of fixing this up on Tuesday (Mon is a holiday here), but if anyone wants to get to it sooner, what I plan to do is:

• Add a new Extension diagnostic about allowing you to define a type in __builtin_offsetof
• Replace the error in C with the extension warning (C++ will continue to err)
• Add documentation to the extensions manual for the builtin
• Update the release note and tests accordingly

[ronlieb]

our nekbone app is failing from this patch.

[fhahn]

In D133574#4053647, @aaron.ballman wrote:

I'll take care of fixing this up on Tuesday (Mon is a holiday here), but if anyone wants to get to it sooner, what I plan to do is:

• Add a new Extension diagnostic about allowing you to define a type in __builtin_offsetof
• Replace the error in C with the extension warning (C++ will continue to err)
• Add documentation to the extensions manual for the builtin
• Update the release note and tests accordingly

It might be worth to revert the patch in the meantime to reduce the impact on adopters that are using current main for testing. Among other things, it also breaks building the gcc workload or SPEC2017

[inclyc]

On Mon, Jan 16, 2023 Roman Lebedev <[e-mail]> wrote:

Reminder to please always mention the reason for the revert/recommit in the commit message.

Thanks! This change needs to be fixed up (see comments above).

[aaron.ballman]

I've relanded this as f1f0a0d8e8fdd2e534d9423b2e64c6b8aaa53aee, changes include:

• Removed error in C mode, turned it into an extension warning
• Added an extension warning for using member access or array subscript expressions for the member designator
• Corrected expected test output
• Added documentation

The new warnings are both under a new flag -Wgnu-offsetof-extensions which is nested under the -Wgnu flag.

Marking this review as accepted so I can close it now that I think it's completed again. However, if anyone spots further concerns, please raise them!

[aaron.ballman]

Closing as completed by f1f0a0d8e8fdd2e534d9423b2e64c6b8aaa53aee

[mstorsjo]

For clarity in the review - the relanded commit ended up reverted by @aeubanks in 39da55e8f548a11f7dadefa73ea73d809a5f1729. The relanded commit triggers failed asserts like this:

$ echo 'void c() { __builtin_offsetof(struct {int b;}, b); }' | bin/clang -cc1 -emit-llvm -o /dev/null - -x c
clang: ../../clang/lib/AST/RecordLayoutBuilder.cpp:3286: const clang::ASTRecordLayout& clang::ASTContext::getASTRecordLayout(const clang::RecordDecl*) const: Assertion `!D->isInvalidDecl() && "Cannot get layout of invalid decl!"' failed.

I also ran into a second issue with the reapplied version of the commit:

$ echo 'int i = __builtin_offsetof(struct {int b;}, b);' | bin/clang -cc1 -emit-llvm -o /dev/null - -x c
<stdin>:1:9: error: initializer element is not a compile-time constant
int i = __builtin_offsetof(struct {int b;}, b);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

(This used to compile successfully before.)

[aaron.ballman]

In D133574#4061428, @mstorsjo wrote:

For clarity in the review - the relanded commit ended up reverted by @aeubanks in 39da55e8f548a11f7dadefa73ea73d809a5f1729. The relanded commit triggers failed asserts like this:

$ echo 'void c() { __builtin_offsetof(struct {int b;}, b); }' | bin/clang -cc1 -emit-llvm -o /dev/null - -x c
clang: ../../clang/lib/AST/RecordLayoutBuilder.cpp:3286: const clang::ASTRecordLayout& clang::ASTContext::getASTRecordLayout(const clang::RecordDecl*) const: Assertion `!D->isInvalidDecl() && "Cannot get layout of invalid decl!"' failed.

I also ran into a second issue with the reapplied version of the commit:

$ echo 'int i = __builtin_offsetof(struct {int b;}, b);' | bin/clang -cc1 -emit-llvm -o /dev/null - -x c
<stdin>:1:9: error: initializer element is not a compile-time constant
int i = __builtin_offsetof(struct {int b;}, b);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

(This used to compile successfully before.)

Sorry for the trouble, there was a declaration being marked as invalid despite us *warning* on it rather than erroring on it. I've corrected it and re-landed in e7300e75b51a7e7d4e81975b4be7a6c65f9a8286 with additional test coverage for both of the scenarios discovered. Thanks!

[asbirlea]

Following this change, the following emits warnings: https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE

Can you please fix fwd or revert until resolved?

[aaron.ballman]

In D133574#4083724, @asbirlea wrote:

Following this change, the following emits warnings: https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE

Can you please fix fwd or revert until resolved?

What do you expect to be resolved? You're passing -Wgnu-offsetof-extensions which is warning you about the extensions, and -Werror is turning them from a warning into an error, so you're getting the behavior I'd expect based on: https://reviews.llvm.org/D133574#4059845

[erichkeane]

In D133574#4083724, @asbirlea wrote:

Following this change, the following emits warnings: https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE

Can you please fix fwd or revert until resolved?

Those both appear to be valid warnings, I don't believe there to be anything to 'fix' or any valid reason to revert here.

[stilor]

In D133574#4083742, @aaron.ballman wrote:

In D133574#4083724, @asbirlea wrote:

Following this change, the following emits warnings: https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE

Can you please fix fwd or revert until resolved?

What do you expect to be resolved? You're passing -Wgnu-offsetof-extensions which is warning you about the extensions, and -Werror is turning them from a warning into an error, so you're getting the behavior I'd expect based on: https://reviews.llvm.org/D133574#4059845

Can you please explain how these warnings are related to the N2350? From what I see in N2350 text, it was only declaring new type definitions inside of offsetof to be undefined behavior. Can you please point to the language in N2350 that prohibits member access expressions like offsetof(x, y.z) or offsetof(x, y[1])?

In fact, a change some time ago explicitly removed the warning for what it called an "non-identifier designator (e.g. offsetof(x, a.b[c])" in response to DR496. The language from DR496 hasn't changed in the most recent publicly available draft of the standard (which includes both DR496 and N2350), so why are these "non-identifier designators" considered extensions now?

[aaron.ballman]

In D133574#4084215, @stilor wrote:

In D133574#4083742, @aaron.ballman wrote:

In D133574#4083724, @asbirlea wrote:

Following this change, the following emits warnings: https://godbolt.org/z/cvGdMWqEa, https://godbolt.org/z/GhTP85WzE

Can you please fix fwd or revert until resolved?

What do you expect to be resolved? You're passing -Wgnu-offsetof-extensions which is warning you about the extensions, and -Werror is turning them from a warning into an error, so you're getting the behavior I'd expect based on: https://reviews.llvm.org/D133574#4059845

Can you please explain how these warnings are related to the N2350? From what I see in N2350 text, it was only declaring new type definitions inside of offsetof to be undefined behavior. Can you please point to the language in N2350 that prohibits member access expressions like offsetof(x, y.z) or offsetof(x, y[1])?

In fact, a change some time ago explicitly removed the warning for what it called an "non-identifier designator (e.g. offsetof(x, a.b[c])" in response to DR496. The language from DR496 hasn't changed in the most recent publicly available draft of the standard (which includes both DR496 and N2350), so why are these "non-identifier designators" considered extensions now?

Thank you for pointing this out, you're right that there's definitely some confusion here and I didn't explain it very well.

DR496 was considered not a defect: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2148.htm#dr_496 and said "There was no discussion asserting that this extension, however, was the actual intent of the standard, and as such there is was no sentiment to accept these extensions with clarified wording. Such a change could only be made in a new revision of the standard.", and the current wording of the C2x standard matches that of the C17 (still says member-designator and not subobject).

From my notes on the discussions of this DR:

Albuquerque 2017: "Next we moved on to DR 496 which is about offsetof. There was an open action item on this which was not written down, but the verbal report was that we should be able to safely talk about subobjects. We will leave this in open status and look at words later."
Pittsburgh 2017: "We did hit a procedural issue when dealing with DR 496 where the information from a previous PCR was not carried forward from one meeting to the next and there was some confusion as to what the "response" really is. We did mention that we want the last committee response to always be the definitive one, rather than making people pull together the history from separate locations. This is the way we usually do it, and this may have just been a process hiccup."
London 2018: "We looked at an open DR for 496 which we elected to mark C2x."

So by my understanding, my original changes removing the extension warning (in D40267) were jumping the gun because the committee never made the change we said we'd make. So I believe this is still an extension as far as C conformance is concerned. That said, I'll check with the convener to see if he'd be too frustrated if I filed a CD2 comment asking for member-designator to be replaced with subobject based on prior discussion, so maayyyybbeee we can fix this for C2x still.

[aaron.ballman]

In D133574#4085372, @aaron.ballman wrote:

So by my understanding, my original changes removing the extension warning (in D40267) were jumping the gun because the committee never made the change we said we'd make. So I believe this is still an extension as far as C conformance is concerned. That said, I'll check with the convener to see if he'd be too frustrated if I filed a CD2 comment asking for member-designator to be replaced with subobject based on prior discussion, so maayyyybbeee we can fix this for C2x still.

I heard back and there's even more confusion -- we were tracking an older copy of the DR list, and there's an update that clarifies this: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_496. So you're right, the array and member access extension warnings need to be removed. I'll take care of that and get it cherry picked into the Clang 16 branch.

[aaron.ballman]

In D133574#4085472, @aaron.ballman wrote:

In D133574#4085372, @aaron.ballman wrote:

So by my understanding, my original changes removing the extension warning (in D40267) were jumping the gun because the committee never made the change we said we'd make. So I believe this is still an extension as far as C conformance is concerned. That said, I'll check with the convener to see if he'd be too frustrated if I filed a CD2 comment asking for member-designator to be replaced with subobject based on prior discussion, so maayyyybbeee we can fix this for C2x still.

I heard back and there's even more confusion -- we were tracking an older copy of the DR list, and there's an update that clarifies this: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_496. So you're right, the array and member access extension warnings need to be removed. I'll take care of that and get it cherry picked into the Clang 16 branch.

I posted https://reviews.llvm.org/D142723 to address this.

[aaron.ballman]

In D133574#4085681, @aaron.ballman wrote:

In D133574#4085472, @aaron.ballman wrote:

In D133574#4085372, @aaron.ballman wrote:

So by my understanding, my original changes removing the extension warning (in D40267) were jumping the gun because the committee never made the change we said we'd make. So I believe this is still an extension as far as C conformance is concerned. That said, I'll check with the convener to see if he'd be too frustrated if I filed a CD2 comment asking for member-designator to be replaced with subobject based on prior discussion, so maayyyybbeee we can fix this for C2x still.

I heard back and there's even more confusion -- we were tracking an older copy of the DR list, and there's an update that clarifies this: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_496. So you're right, the array and member access extension warnings need to be removed. I'll take care of that and get it cherry picked into the Clang 16 branch.

I posted https://reviews.llvm.org/D142723 to address this.

and 63d6b8be6cf2422228a1a8800d85a11be469c6e2 should hopefully resolve it.

[stilor]

I posted https://reviews.llvm.org/D142723 to address this.

and 63d6b8be6cf2422228a1a8800d85a11be469c6e2 should hopefully resolve it.

Thank you for sorting this out so quickly!

[aaron.ballman]

In D133574#4086450, @stilor wrote:

I posted https://reviews.llvm.org/D142723 to address this.

and 63d6b8be6cf2422228a1a8800d85a11be469c6e2 should hopefully resolve it.

Thank you for sorting this out so quickly!

Any time, thank you for raising the issue so quickly!

clang/docs/ReleaseNotes.rst

* When using ``-shared``, the clang driver now invokes llvm-nm to create an
   alternative export control option.
 
 C Language Changes in Clang
 ---------------------------
 - Adjusted ``-Wformat`` warnings according to `WG14 N2562 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2562.pdf>`_.
   Clang will now consider default argument promotions in ``printf``, and remove
   unnecessary warnings. Especially ``int`` argument with specifier ``%hhd`` and
   ``%hd``.
 
 C2x Feature Support
 -------------------
 - Implemented `WG14 N2662 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2662.pdf>`_,
   so the [[maybe_unused]] attribute may be applied to a label to silence an
   ``-Wunused-label`` warning.
 - Implemented `WG14 N2508 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2508.pdf>`_,
   so labels can placed everywhere inside a compound statement.
 - Implemented `WG14 N2927 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2927.htm>`_,
   the Not-so-magic ``typeof`` operator. Also implemented
- Implemented `WG14 N2975 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2975.pdf>`_,
 
   .. code-block:: c
 
     void func(...) {  // Invalid in C17 and earlier, valid in C2x and later.
       va_list list;
       va_start(list); // Invalid in C17 and earlier, valid in C2x and later.
       va_end(list);
     }
     
+- Reject type definitions in the ``type`` argument of ``__builtin_offsetof`` 
+  according to `WG14 N2350 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm>`_.
+
 C++ Language Changes in Clang
 -----------------------------
 - Implemented `DR692 <https://wg21.link/cwg692>`_, `DR1395 <https://wg21.link/cwg1395>`_,
   and `DR1432 <https://wg21.link/cwg1432>`_. The fix for DR1432 is speculative since the
   issue is still open and has no proposed resolution at this time. A speculative fix
   for DR1432 is needed to prevent regressions that would otherwise occur due to DR692.
 - Clang's default C++/ObjC++ standard is now ``gnu++17`` instead of ``gnu++14``.
   This means Clang will by default accept code using features from C++17 and

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_type_defined_in_param_type : Error<
   "%0 cannot be defined in a parameter type">;
 def err_type_defined_in_alias_template : Error<
   "%0 cannot be defined in a type alias template">;
 def err_type_defined_in_condition : Error<
   "%0 cannot be defined in a condition">;
 def err_type_defined_in_enum : Error<
   "%0 cannot be defined in an enumeration">;
+def err_type_defined_in_offsetof : Error<
+  "%0 cannot be defined in '%select{__builtin_offsetof|offsetof}1'">;
 
 def note_pure_virtual_function : Note<
   "unimplemented pure virtual method %0 in %1">;
 
 def note_pure_qualified_call_kext : Note<
   "qualified call to %0::%1 is treated as a virtual call to %1 due to -fapple-kext">;
 
 def err_deleted_decl_not_first : Error<

clang/include/clang/Parse/Parser.h

 /// Parser - This implements a parser for the C family of languages.  After
 /// parsing units of the grammar, productions are invoked to handle whatever has
 /// been read.
 ///
 class Parser : public CodeCompletionHandler {
   friend class ColonProtectionRAIIObject;
   friend class ParsingOpenMPDirectiveRAII;
   friend class InMessageExpressionRAIIObject;
+  friend class OffsetOfStateRAIIObject;
   friend class PoisonSEHIdentifiersRAIIObject;
   friend class ObjCDeclContextSwitch;
   friend class ParenBraceBracketBalancer;
   friend class BalancedDelimiterTracker;
 
   Preprocessor &PP;
 
   /// Tok - The current token we are peeking ahead.  All parsing methods assume
class Parser : public CodeCompletionHandler {
   /// should not be set directly.
   bool InMessageExpression;
 
   /// Gets set to true after calling ProduceSignatureHelp, it is for a
   /// workaround to make sure ProduceSignatureHelp is only called at the deepest
   /// function call.
   bool CalledSignatureHelp = false;
 
+  Sema::OffsetOfKind OffsetOfState = Sema::OffsetOfKind::OOK_Outside;
+
   /// The "depth" of the template parameters currently being parsed.
   unsigned TemplateParameterDepth;
 
   /// Current kind of OpenMP clause
   OpenMPClauseKind OMPClauseKind = llvm::omp::OMPC_unknown;
 
   /// RAII class that manages the template parameter depth.
   class TemplateParameterDepthRAII {
     unsigned &Depth;
     unsigned AddedLevels;
   public:
     explicit TemplateParameterDepthRAII(unsigned &Depth)
       : Depth(Depth), AddedLevels(0) {}
private:
   }
 
   /// Is this a context in which an opaque-enum-declaration can appear?
   static bool isOpaqueEnumDeclarationContext(DeclSpecContext DSC) {
     switch (DSC) {
     case DeclSpecContext::DSC_normal:
     case DeclSpecContext::DSC_class:
     case DeclSpecContext::DSC_top_level:
       return true;
 
     case DeclSpecContext::DSC_alias_declaration:
     case DeclSpecContext::DSC_objc_method_result:
     case DeclSpecContext::DSC_condition:
     case DeclSpecContext::DSC_template_param:
     case DeclSpecContext::DSC_template_type_arg:
     case DeclSpecContext::DSC_type_specifier:
     case DeclSpecContext::DSC_trailing:
static bool isClassTemplateDeductionContext(DeclSpecContext DSC) {
     case DeclSpecContext::DSC_normal:
     case DeclSpecContext::DSC_template_param:
     case DeclSpecContext::DSC_template_arg:
     case DeclSpecContext::DSC_class:
     case DeclSpecContext::DSC_top_level:
     case DeclSpecContext::DSC_condition:
     case DeclSpecContext::DSC_type_specifier:
     case DeclSpecContext::DSC_association:
     case DeclSpecContext::DSC_conv_operator:
       return true;
 
     case DeclSpecContext::DSC_objc_method_result:
     case DeclSpecContext::DSC_template_type_arg:
     case DeclSpecContext::DSC_trailing:
     case DeclSpecContext::DSC_alias_declaration:
       return false;
     }

clang/include/clang/Parse/RAIIObjectsForParser.h

InMessageExpressionRAIIObject(Parser &P, bool Value)
       InMessageExpression = Value;
     }
 
     ~InMessageExpressionRAIIObject() {
       InMessageExpression = OldValue;
     }
   };
 
+  class OffsetOfStateRAIIObject {
+    Sema::OffsetOfKind &OffsetOfState;
+    Sema::OffsetOfKind OldValue;
+
+  public:
+    OffsetOfStateRAIIObject(Parser &P, Sema::OffsetOfKind Value)
+        : OffsetOfState(P.OffsetOfState), OldValue(P.OffsetOfState) {
+      OffsetOfState = Value;
+    }
+
+    ~OffsetOfStateRAIIObject() { OffsetOfState = OldValue; }
+  };
+
   /// RAII object that makes sure paren/bracket/brace count is correct
   /// after declaration/statement parsing, even when there's a parsing error.
   class ParenBraceBracketBalancer {
     Parser &P;
     unsigned short ParenCount, BracketCount, BraceCount;
   public:
     ParenBraceBracketBalancer(Parser &p)
       : P(p), ParenCount(p.ParenCount), BracketCount(p.BracketCount),

clang/include/clang/Sema/Sema.h

public:
 
   enum TagUseKind {
     TUK_Reference,   // Reference to a tag:  'struct foo *X;'
     TUK_Declaration, // Fwd decl of a tag:   'struct foo;'
     TUK_Definition,  // Definition of a tag: 'struct foo { int X; } Y;'
     TUK_Friend       // Friend declaration:  'friend struct foo;'
   };
 
+  enum OffsetOfKind {
+    // Not parsing a type within __builtin_offsetof.
+    OOK_Outside,
+    // Parsing a type within __builtin_offsetof.
+    OOK_Builtin,
+    // Parsing a type within macro "offsetof", defined in __buitin_offsetof
+    // To improve our diagnostic message.
+    OOK_Macro,
+  };
+
   Decl *ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                  SourceLocation KWLoc, CXXScopeSpec &SS, IdentifierInfo *Name,
                  SourceLocation NameLoc, const ParsedAttributesView &Attr,
                  AccessSpecifier AS, SourceLocation ModulePrivateLoc,
                  MultiTemplateParamsArg TemplateParameterLists, bool &OwnedDecl,
                  bool &IsDependent, SourceLocation ScopedEnumKWLoc,
                  bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
                  bool IsTypeSpecifier, bool IsTemplateParamOrArg,
-                 SkipBodyInfo *SkipBody = nullptr);
+                 OffsetOfKind OOK, SkipBodyInfo *SkipBody = nullptr);
 
   Decl *ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
                                 unsigned TagSpec, SourceLocation TagLoc,
                                 CXXScopeSpec &SS, IdentifierInfo *Name,
                                 SourceLocation NameLoc,
                                 const ParsedAttributesView &Attr,
                                 MultiTemplateParamsArg TempParamLists);
 

clang/lib/Parse/ParseDecl.cpp

void Parser::ParseEnumSpecifier(SourceLocation StartLoc, DeclSpec &DS,
   unsigned DiagID;
   Decl *TagDecl = Actions.ActOnTag(
       getCurScope(), DeclSpec::TST_enum, TUK, StartLoc, SS, Name, NameLoc,
       attrs, AS, DS.getModulePrivateSpecLoc(), TParams, Owned, IsDependent,
       ScopedEnumKWLoc, IsScopedUsingClassTag, BaseType,
       DSC == DeclSpecContext::DSC_type_specifier,
       DSC == DeclSpecContext::DSC_template_param ||
           DSC == DeclSpecContext::DSC_template_type_arg,
-      &SkipBody);
+      OffsetOfState, &SkipBody);
 
   if (SkipBody.ShouldSkip) {
     assert(TUK == Sema::TUK_Definition && "can only skip a definition");
 
     BalancedDelimiterTracker T(*this, tok::l_brace);
     T.consumeOpen();
     T.skipToEnd();
 

clang/lib/Parse/ParseDeclCXX.cpp

if (TemplateId) {
     // Declaration or definition of a class type
     TagOrTempResult = Actions.ActOnTag(
         getCurScope(), TagType, TUK, StartLoc, SS, Name, NameLoc, attrs, AS,
         DS.getModulePrivateSpecLoc(), TParams, Owned, IsDependent,
         SourceLocation(), false, clang::TypeResult(),
         DSC == DeclSpecContext::DSC_type_specifier,
         DSC == DeclSpecContext::DSC_template_param ||
             DSC == DeclSpecContext::DSC_template_type_arg,
-        &SkipBody);
+        OffsetOfState, &SkipBody);
 
     // If ActOnTag said the type was dependent, try again with the
     // less common call.
     if (IsDependent) {
       assert(TUK == Sema::TUK_Reference || TUK == Sema::TUK_Friend);
       TypeResult = Actions.ActOnDependentTag(getCurScope(), TagType, TUK, SS,
                                              Name, StartLoc, NameLoc);
     }

clang/lib/Parse/ParseExpr.cpp

case tok::kw___builtin_va_arg: {
     if (Expr.isInvalid() || Ty.isInvalid())
       Res = ExprError();
     else
       Res = Actions.ActOnVAArg(StartLoc, Expr.get(), Ty.get(), ConsumeParen());
     break;
   }
   case tok::kw___builtin_offsetof: {
     SourceLocation TypeLoc = Tok.getLocation();
-    TypeResult Ty = ParseTypeName();
+    auto K = Sema::OffsetOfKind::OOK_Builtin;
+    if (Tok.getLocation().isMacroID()) {
+      StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
+          Tok.getLocation(), PP.getSourceManager(), getLangOpts());
+      if (MacroName == "offsetof")
+        K = Sema::OffsetOfKind::OOK_Macro;
+    }
+    TypeResult Ty;
+    {
+      OffsetOfStateRAIIObject InOffsetof(*this, K);
+      Ty = ParseTypeName();
       if (Ty.isInvalid()) {
         SkipUntil(tok::r_paren, StopAtSemi);
         return ExprError();
       }
+    }
 
     if (ExpectAndConsume(tok::comma)) {
       SkipUntil(tok::r_paren, StopAtSemi);
       return ExprError();
     }
 
     // We must have at least one identifier here.
     if (Tok.isNot(tok::identifier)) {

clang/lib/Sema/SemaDecl.cpp

Decl *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK,
                      IdentifierInfo *Name, SourceLocation NameLoc,
                      const ParsedAttributesView &Attrs, AccessSpecifier AS,
                      SourceLocation ModulePrivateLoc,
                      MultiTemplateParamsArg TemplateParameterLists,
                      bool &OwnedDecl, bool &IsDependent,
                      SourceLocation ScopedEnumKWLoc,
                      bool ScopedEnumUsesClassTag, TypeResult UnderlyingType,
                      bool IsTypeSpecifier, bool IsTemplateParamOrArg,
-                     SkipBodyInfo *SkipBody) {
+                     OffsetOfKind OOK, SkipBodyInfo *SkipBody) {
   // If this is not a definition, it must have a name.
   IdentifierInfo *OrigName = Name;
   assert((Name != nullptr || TUK == TUK_Definition) &&
          "Nameless record must be a definition!");
   assert(TemplateParameterLists.size() == 0 || TUK != TUK_Reference);
 
   OwnedDecl = false;
   TagTypeKind Kind = TypeWithKeyword::getTagTypeKindForTypeSpec(TagSpec);
if (getLangOpts().CPlusPlus) {
 
       if (isStdBadAlloc && (!StdBadAlloc || getStdBadAlloc()->isImplicit()))
         StdBadAlloc = cast<CXXRecordDecl>(New);
     } else
       New = RecordDecl::Create(Context, Kind, SearchDC, KWLoc, Loc, Name,
                                cast_or_null<RecordDecl>(PrevDecl));
   }
 
+  if (OOK != OOK_Outside && TUK == TUK_Definition) {
+    Diag(New->getLocation(), diag::err_type_defined_in_offsetof)
+        << Context.getTagDeclType(New) << static_cast<int>(OOK == OOK_Macro);
+    Invalid = true;
+  }
+
   // C++11 [dcl.type]p3:
   //   A type-specifier-seq shall not define a class or enumeration [...].
-  if (getLangOpts().CPlusPlus && (IsTypeSpecifier || IsTemplateParamOrArg) &&
-      TUK == TUK_Definition) {
+  if (!Invalid && getLangOpts().CPlusPlus &&
+      (IsTypeSpecifier || IsTemplateParamOrArg) && TUK == TUK_Definition) {
     Diag(New->getLocation(), diag::err_type_defined_in_type_specifier)
       << Context.getTagDeclType(New);
     Invalid = true;
   }
 
   if (!Invalid && getLangOpts().CPlusPlus && TUK == TUK_Definition &&
       DC->getDeclKind() == Decl::Enum) {
     Diag(New->getLocation(), diag::err_type_defined_in_enum)

clang/lib/Sema/SemaDeclCXX.cpp

Decl *Sema::ActOnTemplatedFriendTag(Scope *S, SourceLocation FriendLoc,
 
   // If it's explicit specializations all the way down, just forget
   // about the template header and build an appropriate non-templated
   // friend.  TODO: for source fidelity, remember the headers.
   if (isAllExplicitSpecializations) {
     if (SS.isEmpty()) {
       bool Owned = false;
       bool IsDependent = false;
-      return ActOnTag(S, TagSpec, TUK_Friend, TagLoc, SS, Name, NameLoc,
-                      Attr, AS_public,
+      return ActOnTag(S, TagSpec, TUK_Friend, TagLoc, SS, Name, NameLoc, Attr,
+                      AS_public,
                       /*ModulePrivateLoc=*/SourceLocation(),
                       MultiTemplateParamsArg(), Owned, IsDependent,
                       /*ScopedEnumKWLoc=*/SourceLocation(),
                       /*ScopedEnumUsesClassTag=*/false,
                       /*UnderlyingType=*/TypeResult(),
                       /*IsTypeSpecifier=*/false,
-                      /*IsTemplateParamOrArg=*/false);
+                      /*IsTemplateParamOrArg=*/false, /*OOK=*/OOK_Outside);
     }
 
     NestedNameSpecifierLoc QualifierLoc = SS.getWithLocInContext(Context);
     ElaboratedTypeKeyword Keyword
       = TypeWithKeyword::getKeywordForTagTypeKind(Kind);
     QualType T = CheckTypenameType(Keyword, TagLoc, QualifierLoc,
                                    *Name, NameLoc);
     if (T.isNull())

clang/lib/Sema/SemaTemplate.cpp

 Sema::ActOnExplicitInstantiation(Scope *S, SourceLocation ExternLoc,
                                  SourceLocation TemplateLoc, unsigned TagSpec,
                                  SourceLocation KWLoc, CXXScopeSpec &SS,
                                  IdentifierInfo *Name, SourceLocation NameLoc,
                                  const ParsedAttributesView &Attr) {
 
   bool Owned = false;
   bool IsDependent = false;
-  Decl *TagD = ActOnTag(S, TagSpec, Sema::TUK_Reference,
-                        KWLoc, SS, Name, NameLoc, Attr, AS_none,
-                        /*ModulePrivateLoc=*/SourceLocation(),
-                        MultiTemplateParamsArg(), Owned, IsDependent,
-                        SourceLocation(), false, TypeResult(),
-                        /*IsTypeSpecifier*/false,
-                        /*IsTemplateParamOrArg*/false);
+  Decl *TagD = ActOnTag(
+      S, TagSpec, Sema::TUK_Reference, KWLoc, SS, Name, NameLoc, Attr, AS_none,
+      /*ModulePrivateLoc=*/SourceLocation(), MultiTemplateParamsArg(), Owned,
+      IsDependent, SourceLocation(), false, TypeResult(),
+      /*IsTypeSpecifier*/ false,
+      /*IsTemplateParamOrArg=*/false, /*OOK=*/OOK_Outside);
   assert(!IsDependent && "explicit instantiation of dependent name not yet handled");
 
   if (!TagD)
     return true;
 
   TagDecl *Tag = cast<TagDecl>(TagD);
   assert(!Tag->isEnum() && "shouldn't see enumerations here");
 

clang/test/C/C2x/n2350.c

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c89 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c99 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c17 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c2x -verify %s
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int simple(void) {
+  return __builtin_offsetof(struct A // expected-error{{'struct A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+    int a;
+    struct B // expected-error{{'struct B' cannot be defined in '__builtin_offsetof'}} 
+    {
+      int c;
+      int d;
+    } x;
+  }, a);
+}
+
+int anonymous_struct() {
+  return __builtin_offsetof(struct // expected-error-re{{'struct (unnamed at {{.*}})' cannot be defined in '__builtin_offsetof'}}
+  { 
+    int a;
+    int b;
+  }, a);
+}
+
+int struct_in_second_param() {
+  struct A {
+    int a, b;
+    int x[20];
+  };
+  return __builtin_offsetof(struct A, x[sizeof(struct B{int a;})]); // no-error
+}
+
+
+#define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
+
+
+int macro(void) {
+  return offsetof(struct A // expected-error{{'struct A' cannot be defined in 'offsetof'}}
+                           // expected-error@-1{{'struct B' cannot be defined in 'offsetof'}}
+  { 
+    int a;
+    struct B // verifier seems to think the error is emitted by the macro
+             // In fact the location of the error is "B" on the line above
+    {
+      int c;
+      int d;
+    } x;
+  }, a);
+}
+
+#undef offsetof
+
+#define offsetof(TYPE, MEMBER) (&((TYPE *)0)->MEMBER)
+
+// no warning for traditional offsetof as a function-like macro
+int * macro_func(void) {
+  return offsetof(struct A // no-warning
+  { 
+    int a;
+    int b;
+  }, a);
+}

clang/test/C/drs/dr4xx.c

void dr496(void) {
    * if that would be a valid expression. FIXME: GCC accepts this, perhaps we
    * should as well.
    */
   (void)__builtin_offsetof(struct C, a->n); /* expected-error {{expected ')'}} \
                                                expected-note {{to match this '('}}
                                              */
 
   /* The DR asked a question about whether defining a new type within offsetof
-   * is allowed. C2x N2350 made this explicitly undefined behavior, but Clang
-   * has always supported defining a type in this location, and GCC also
-   * supports it.
+   * is allowed. C2x N2350 made this explicitly undefined behavior, but GCC
+   * supports it, Clang diagnoses this a UB and rejects it.
    */
-   (void)__builtin_offsetof(struct S { int a; }, a);
+   (void)__builtin_offsetof(struct S { int a; }, a); /* expected-error{{'struct S' cannot be defined in '__builtin_offsetof'}} */
 }
 
 /* WG14 DR499: yes
  * Anonymous structure in union behavior
  */
 void dr499(void) {
   union U {
     struct {

clang/test/Parser/declarators.c

struct test9 {
   int y;
   int z  // expected-warning {{expected ';' at end of declaration list}}
 };
 
 // PR6208
 struct test10 { int a; } static test10x;
 struct test11 { int a; } const test11x;
 
-// PR6216
-void test12(void) {
-  (void)__builtin_offsetof(struct { char c; int i; }, i);
-}
 
 // rdar://7608537
 struct test13 { int a; } (test13x);
 
 // <rdar://problem/8044088>
 struct X<foo::int> { }; // expected-error{{expected identifier or '('}}
 
 

clang/test/SemaCXX/offsetof.cpp

 struct Base {
   int Field;
 };
 struct Derived : virtual Base {
   void Fun() { (void)__builtin_offsetof(Derived, Field); } // expected-warning {{offset of on non-POD type}} \
                                                               expected-error {{invalid application of 'offsetof' to a field of a virtual base}}
 };
 }
+
+// Reject definitions in __builtin_offsetof
+// https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2350.htm
+int test_definition(void) {
+  return __builtin_offsetof(struct A // expected-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+  { 
+    int a;
+    struct B // FIXME: error diagnostic message for nested definitions 
+             // https://reviews.llvm.org/D133574 
+             // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}} 
+    {
+      int c;
+      int d;
+    };
+    B x;
+  }, a);
+}