[Clang] Support qualified name as member designator in offsetof

[yichi170]

Summary

fix: https://github.com/llvm/llvm-project/issues/64154

[tbaeder]

You're missing tests.

clang/lib/Sema/SemaExpr.cpp

16696–16697

[cor3ntin]

Thanks a lot for working on this. Can you add tests (to clang/test/SemaCXX/offsetof.cpp)?
The change will also need an entry in clang/docs/ReleaseNotes.rst , in the "C++ Language Changes" section

[yichi170]

I updated the test! Please check it out!
Should I update which subsection in "C++ Language Changes"?

[yichi170]

clang/lib/Sema/SemaExpr.cpp

16696–16697

I'll fix it along with the ReleaseNotes.

[Fznamznon]

clang/test/Sema/offsetof.c

79 ↗

(On Diff #547542)

It probably makes sense to also add a test with static member of a class, like Aaron mentioned in GitHub - https://github.com/llvm/llvm-project/issues/64154#issuecomment-1653468938 .

[yichi170]

updated ReleaseNotes and applied suggestions. The related issue #63443 is still not fixed.

[yichi170]

clang/test/Sema/offsetof.c

79 ↗

(On Diff #547542)

Hi @Fznamznon, I found that clang and clang++ output different errors with the following test.

struct X2 { int a; static int static_a; };
int x4[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1];
// expected-error{{no member named 'static_a'}}

C doesn't allow to have a static member in the struct, so is there any way to write the correct comment to make the test pass?

[cor3ntin]

clang/test/Sema/offsetof.c

79 ↗

(On Diff #547542)

the tests should be in test/SemaCXX/offsetof.cpp, where you can use C++

[yichi170]

updated test in test/SemaCXX/offsetof.cpp

[aaron.ballman]

Thank you for working on this!

clang/docs/ReleaseNotes.rst

59 ↗

(On Diff #547764)

You should also link to https://github.com/llvm/llvm-project/issues/64154 here to tie it back to a bug report.

clang/include/clang/Sema/Sema.h

6038–6039

I think we should combine isBrackets and isQualifier since a component can't be both at the same time anyway. e.g.,

enum {
  Brackets,   // U.E is valid
  Identifier, // U.IdentInfo is valid
  Qualifier,  // Nothing in U is valid
} Kind;

clang/test/Sema/offsetof.c

77–79 ↗

(On Diff #547764)

Neither of these should be valid in C -- there is no way to name subobjects in C like there is in C++.

clang/test/SemaCXX/offsetof.cpp

104 ↗

(On Diff #547764)

It'd be good to add the int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1]; test here from the C test.

[yichi170]

Applied the suggestions. Thanks for giving me feedback!

[cor3ntin]

clang/include/clang/Sema/Sema.h

6038–6039

This are not the best names.

ArrayElement, Subobject, NestedNameQualifier

Are probably more descriptive.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

59 ↗

(On Diff #548632)

clang/lib/Parse/ParseExpr.cpp

2646–2651

I think this is slightly more clear.

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

There's one more test I'd like to see:

struct S {
  int Foo;
};

template <typename Ty>
void func() {
  static_assert(__builtin_offsetof(Ty, Ty::Foo) == 0, "");
}

void inst() {
  func<S>();
}

[yichi170]

clang/docs/ReleaseNotes.rst

59 ↗

(On Diff #548632)

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

It would get the compile error in the current patch, but I think it should be compiled without any error, right?

[aaron.ballman]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

Correct, that should be accepted: https://godbolt.org/z/1f6a9Yaxa

[hubert.reinterpretcast]

clang/test/SemaCXX/offsetof.cpp

104 ↗

(On Diff #548632)

Tests should include cases where the member is a direct member of a base class or an anonymous union therein (and named variously by the qualification with the base class name, the complete class name, an intermediate base class name, the base class name qualified with an intermediate base class, the base class name denoted using ::identity_t<Base>, etc.).

Additionally, there should be a diagnostic test case where the qualifier is for an unrelated identically-defined class in another namespace.

[hubert.reinterpretcast]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

Should expect this to pass too:

template <typename T>
struct Z {
  static_assert(!__builtin_offsetof(T, template Q<T>::x));
};

struct A {
  template <typename T> using Q = T;
  int x;
};

Z<A> za;

[yichi170]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

Wow. Does it mean we cannot simply parse the identifier, ::, . and brackets in __builtin_offsetof?

[yichi170]

Is there any code handling the nested qualifier that I can reference? I tried to modify the OffsetOfNode, which allows me to handle the basic template case but still failed on the nested case.

[cor3ntin]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

GCC seems to support that.

We probably want to call ParseOptionalCXXScopeSpecifier and store the NestedNameSpecifierLoc we'd get from it (and then parse the (sequence of) identifier(s) corresponding to the member) as we do now.

The documentation of https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Offsetof.html#index-_005f_005fbuiltin_005foffsetof
seems inaccurate,

it seems to be

"__builtin_offsetof" "(" typename "," nested-name-specifier offsetof_member_designator ")"

Note that you will have to take care of transforming the nested name in TreeTransform and handle type dependencies. Let me know if you have further questions,
it's more involved than what you signed for. Sorry for not spotting that earlier (Thanks @hubert.reinterpretcast !)

[yichi170]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

Thank you for all the help! I'll take a look at it!

[cor3ntin]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

I was wrong, we need another approach.

I think the grammar is actually

member-designator:
          qualified-id
          member-designator.qualified-id
          member-designator.qualified-id

IE, we should support https://godbolt.org/z/eEq8snMc8

Unfortunately, this looks like a much bigger change that we envisioned when we tagged this as a good first issue, to the extent I'm not sure what is actually the right design is would be.

For each component I imagine we want to store
NestedNameSpecifierLoc + DeclarationNameInfo

The parser would have to produce a CXXScopeSpec + UnqualifiedId pair for each component.

The expression is dependent if any of the component is type dependent,

OffsetOfNode would have to change, but i think we can get away
Only changing the identifier case (ie the dependent case)

[yichi170]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

Would it be better for me to close this patch and submit a new one if I find out how to implement it? I hope others won't hesitate to contribute because I'm working on this. I don't want to cause any delays in the release plan!

[Endill]

clang/test/SemaCXX/offsetof.cpp

106 ↗

(On Diff #548632)

Would it be better for me to close this patch and submit a new one if I find out how to implement it?

A possible approach is to follow the example of member reference expression in its dot form.

I hope others won't hesitate to contribute because I'm working on this. I don't want to cause any delays in the release plan!

No worries, 17 release has branched a month ago, so you don't have to feel stressed over that.

clang/include/clang/Sema/Sema.h

ExprResult BuildStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
                            SourceLocation RPLoc, unsigned TemplateDepth);
   // Handle the final expression in a statement expression.
   ExprResult ActOnStmtExprResult(ExprResult E);
   void ActOnStmtExprError();
 
   // __builtin_offsetof(type, identifier(.identifier|[expr])*)
   struct OffsetOfComponent {
     SourceLocation LocStart, LocEnd;
     bool isBrackets;  // true if [expr], false if .ident
+    bool isQualifier;
     union {
       IdentifierInfo *IdentInfo;
       Expr *E;
     } U;
   };
 
   /// __builtin_offsetof(type, a.b[123][456].c)
   ExprResult BuildBuiltinOffsetOf(SourceLocation BuiltinLoc,

clang/lib/Parse/ParseExpr.cpp

case tok::kw___builtin_offsetof: {
 
     Comps.push_back(Sema::OffsetOfComponent());
     Comps.back().isBrackets = false;
     Comps.back().U.IdentInfo = Tok.getIdentifierInfo();
     Comps.back().LocStart = Comps.back().LocEnd = ConsumeToken();
 
     // FIXME: This loop leaks the index expressions on error.
     while (true) {
-      if (Tok.is(tok::period)) {
+      if (Tok.is(tok::period) || Tok.is(tok::coloncolon)) {
         // offsetof-member-designator: offsetof-member-designator '.' identifier
+        if (Tok.is(tok::coloncolon))
+          Comps.back().isQualifier = true;
         Comps.push_back(Sema::OffsetOfComponent());
         Comps.back().isBrackets = false;
+        Comps.back().isQualifier = false;
         Comps.back().LocStart = ConsumeToken();
 
         if (Tok.isNot(tok::identifier)) {
           Diag(Tok, diag::err_expected) << tok::identifier;
           SkipUntil(tok::r_paren, StopAtSemi);
           return ExprError();
         }
         Comps.back().U.IdentInfo = Tok.getIdentifierInfo();
         Comps.back().LocEnd = ConsumeToken();
       } else if (Tok.is(tok::l_square)) {
         if (CheckProhibitedCXX11Attribute())
           return ExprError();
 
         // offsetof-member-designator: offsetof-member-design '[' expression ']'
         Comps.push_back(Sema::OffsetOfComponent());
         Comps.back().isBrackets = true;
+        Comps.back().isQualifier = false;
         BalancedDelimiterTracker ST(*this, tok::l_square);
         ST.consumeOpen();
         Comps.back().LocStart = ST.getOpenLocation();
         Res = ParseExpression();
         if (Res.isInvalid()) {
           SkipUntil(tok::r_paren, StopAtSemi);
           return Res;
         }

clang/lib/Sema/SemaExpr.cpp

for (const OffsetOfComponent &OC : Components) {
     // Look for the field.
     LookupResult R(*this, OC.U.IdentInfo, OC.LocStart, LookupMemberName);
     LookupQualifiedName(R, RD);
     FieldDecl *MemberDecl = R.getAsSingle<FieldDecl>();
     IndirectFieldDecl *IndirectMemberDecl = nullptr;
     if (!MemberDecl) {
       if ((IndirectMemberDecl = R.getAsSingle<IndirectFieldDecl>()))
         MemberDecl = IndirectMemberDecl->getAnonField();
+
+      IdentifierInfo *II = RD->getIdentifier();
+      if (II == OC.U.IdentInfo && OC.isQualifier)
+        continue;
     }
 
     if (!MemberDecl) {
       // Lookup could be ambiguous when looking up a placeholder variable
       // __builtin_offsetof(S, _).
       // In that case we would already have emitted a diagnostic
       if (!R.isAmbiguous())
         Diag(BuiltinLoc, diag::err_no_member)