[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

16700–16701

[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

16700–16701

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

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

6037–6038

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

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

6037–6038

This are not the best names.

ArrayElement, Subobject, NestedNameQualifier

Are probably more descriptive.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

59

clang/lib/Parse/ParseExpr.cpp

2646–2651

I think this is slightly more clear.

clang/test/SemaCXX/offsetof.cpp

106

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

clang/test/SemaCXX/offsetof.cpp

106

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

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

[hubert.reinterpretcast]

clang/test/SemaCXX/offsetof.cpp

104

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

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

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

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

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

[cor3ntin]

clang/test/SemaCXX/offsetof.cpp

106

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

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

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/docs/ReleaseNotes.rst

 ==============================
 Some of the major new features and improvements to Clang are listed
 here. Generic improvements to Clang as a whole or to its underlying
 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 C++ Language Changes
 --------------------
+- Improved ``__builtin_offsetof`` support, allowing qualified name in member designator. This fixes [Issue 64154](https://github.com/llvm/llvm-project/issues/64154).
 
 C++20 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 C++23 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 C++2c Feature Support

clang/include/clang/Sema/Sema.h

public:
   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
     union {
       IdentifierInfo *IdentInfo;
       Expr *E;
     } U;
+    enum {
+      Brackets,   // U.E is valid
+      Identifier, // U.IdentInfo is valid
+      Qualifier,  // Nothing in U is valid
+    } Kind;
   };
 
   /// __builtin_offsetof(type, a.b[123][456].c)
   ExprResult BuildBuiltinOffsetOf(SourceLocation BuiltinLoc,
                                   TypeSourceInfo *TInfo,
                                   ArrayRef<OffsetOfComponent> Components,
                                   SourceLocation RParenLoc);
   ExprResult ActOnBuiltinOffsetOf(Scope *S,

clang/lib/Parse/ParseExpr.cpp

if (Tok.isNot(tok::identifier)) {
       SkipUntil(tok::r_paren, StopAtSemi);
       return ExprError();
     }
 
     // Keep track of the various subcomponents we see.
     SmallVector<Sema::OffsetOfComponent, 4> Comps;
 
     Comps.push_back(Sema::OffsetOfComponent());
-    Comps.back().isBrackets = false;
+    Comps.back().Kind = Sema::OffsetOfComponent::Identifier;
     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) && getLangOpts().CPlusPlus) {
+          Comps.back().Kind = Sema::OffsetOfComponent::Qualifier;
+        } else if (Tok.is(tok::coloncolon) && !getLangOpts().CPlusPlus) {
+          Res = ExprError();
+          break;
+        }
+
         Comps.push_back(Sema::OffsetOfComponent());
-        Comps.back().isBrackets = false;
+        Comps.back().Kind = Sema::OffsetOfComponent::Identifier;
         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().Kind = Sema::OffsetOfComponent::Brackets;
         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

if (!Dependent
                              diag::err_offsetof_incomplete_type, TypeRange))
     return ExprError();
 
   bool DidWarnAboutNonPOD = false;
   QualType CurrentType = ArgTy;
   SmallVector<OffsetOfNode, 4> Comps;
   SmallVector<Expr*, 4> Exprs;
   for (const OffsetOfComponent &OC : Components) {
-    if (OC.isBrackets) {
+    if (OC.Kind == OffsetOfComponent::Brackets) {
       // Offset of an array sub-field.  TODO: Should we allow vector elements?
       if (!CurrentType->isDependentType()) {
         const ArrayType *AT = Context.getAsArrayType(CurrentType);
         if(!AT)
           return ExprError(Diag(OC.LocEnd, diag::err_offsetof_array_type)
                            << CurrentType);
         CurrentType = AT->getElementType();
       } else
if (CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD)) {
                             : diag::ext_offsetof_non_pod_type;
 
       if (!IsSafe && !DidWarnAboutNonPOD &&
           DiagRuntimeBehavior(BuiltinLoc, nullptr,
                               PDiag(DiagID)
                               << SourceRange(Components[0].LocStart, OC.LocEnd)
                               << CurrentType))
         DidWarnAboutNonPOD = true;
+
+      if (OC.Kind == OffsetOfComponent::Qualifier &&
+          RD->getIdentifier() == OC.U.IdentInfo)
+        continue;
     }
 
     // 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();
     }
 
     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)
             << OC.U.IdentInfo << RD << SourceRange(OC.LocStart, OC.LocEnd);
       return ExprError();
     }

clang/lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformOffsetOfExpr(OffsetOfExpr *E) {
   // the fields again. However, __builtin_offsetof is rare enough in
   // template code that we don't care.
   bool ExprChanged = false;
   typedef Sema::OffsetOfComponent Component;
   SmallVector<Component, 4> Components;
   for (unsigned I = 0, N = E->getNumComponents(); I != N; ++I) {
     const OffsetOfNode &ON = E->getComponent(I);
     Component Comp;
-    Comp.isBrackets = true;
+    Comp.Kind = Sema::OffsetOfComponent::Brackets;
     Comp.LocStart = ON.getSourceRange().getBegin();
     Comp.LocEnd = ON.getSourceRange().getEnd();
     switch (ON.getKind()) {
     case OffsetOfNode::Array: {
       Expr *FromIndex = E->getIndexExpr(ON.getArrayExprIndex());
       ExprResult Index = getDerived().TransformExpr(FromIndex);
       if (Index.isInvalid())
         return ExprError();
 
       ExprChanged = ExprChanged || Index.get() != FromIndex;
-      Comp.isBrackets = true;
+      Comp.Kind = Sema::OffsetOfComponent::Brackets;
       Comp.U.E = Index.get();
       break;
     }
 
     case OffsetOfNode::Field:
     case OffsetOfNode::Identifier:
-      Comp.isBrackets = false;
+      Comp.Kind = Sema::OffsetOfComponent::Identifier;
       Comp.U.IdentInfo = ON.getFieldName();
       if (!Comp.U.IdentInfo)
         continue;
 
       break;
 
     case OffsetOfNode::Base:
       // Will be recomputed during the rebuild.

clang/test/SemaCXX/offsetof.cpp

struct B // FIXME: error diagnostic message for nested definitions
              // fixme-error{{'A' cannot be defined in '__builtin_offsetof'}}
     {
       int c;
       int d;
     };
     B x;
   }, a);
 }
+
+// https://github.com/llvm/llvm-project/issues/64154
+struct X2 { int a; static int static_a; };
+int x2[__builtin_offsetof(struct X2, X2::a) == 0 ? 1 : -1];
+int x3[__builtin_offsetof(struct X2, X2::static_a) == 0 ? 1 : -1]; // expected-error{{no member named 'static_a'}}
+int x4[__builtin_offsetof(struct X2, X2::X2) == 0 ? 1 : -1]; // expected-error{{no member named 'X2'}}
+