[clang] Make predefined expressions string literals under -fms-extensions

[aeubanks]

Summary

MSVC makes these string literals [1][2].

[1] https://godbolt.org/z/6vnTzbExx
[2] https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170

Fixes #114

[hans]

+mstorsjo is this okay for mingw mode too?

clang/lib/Sema/SemaExpr.cpp

3556

Since this becomes an "else after return" it could be dropped.

clang/test/CodeGen/predefined-expr.c

4 ↗

(On Diff #507896)

Both invocations use the Itanium abi, so the check prefix is a bit confusing. How about DEFAULT and MS?

clang/test/Sema/ms_predefined_expr.cpp

5

ultra nit: the 'void' param makes this look like c code. Should it go in a .c file instead of .cpp?

clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

1062 ↗

(On Diff #507896)

Could the same check be used in ASTImporterTest.cpp?

[aaron.ballman]

This should also have a release note, eventually.

clang/lib/Sema/SemaExpr.cpp

3583–3588

This is incorrect -- these are still predefined expressions even if they're a string literal. Otherwise, we lose AST fidelity and things like the predefinedExpr() AST matcher don't work (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/ASTMatchers/ASTMatchers.h#L2697), and -ast-print will print the wrong thing (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/StmtPrinter.cpp#L1248).

[mstorsjo]

In D146764#4219278, @hans wrote:

+mstorsjo is this okay for mingw mode too?

I believe the current form of the patch is fine - i.e. disabled by default in mingw mode, but enabled if the extra MS language extensions are enabled. (I didn't check myself how those options map to LangOpts.MicrosoftExt but I presume it works as I described above.)

clang/include/clang/Testing/TestClangConfig.h

61 ↗

(On Diff #507896)

I'm not entirely sure of the use context of this function (only specific tests?), but mingw targets with a differen triple are win32 too. But then they could also be using an entirely different architecture than x86_64, so I guess this might be ok too if we're ready to tweak it when a need arises?

[aeubanks]

clang/lib/Sema/SemaExpr.cpp

3583–3588

how would you structure this? an almost identical PredefinedExpr class that also subclasses StringLiteral? or special case all the places that check for StringLiteral to also check for PredefinedExpr (the case I was mostly looking at was initializing a char array with predefined expressions but presumably there are more places that matter)

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

3583–3588

We can't inherit from StringLiteral (it's a final class because it has TrailingObjects), so that might be the only viable approach. However, there's a fair number of places in the compiler where we care if something is or isn't a StringLiteral, so I can see why we'd want to avoid that if possible.

Oye, but it seems that even MSVC is not consistent with whether the predefined expression is a string literal or not: https://godbolt.org/z/jzY8cz79d

[cor3ntin]

clang/lib/Sema/SemaExpr.cpp

3583–3588

I think these should be plain old string literals, but for the purpose or AST matchers and such, maybe we could have a "predefined macro" bit, or even an additional object that stores the name.

[aeubanks]

clang/lib/Sema/SemaExpr.cpp

3583–3588

Oye, but it seems that even MSVC is not consistent with whether the predefined expression is a string literal or not: https://godbolt.org/z/jzY8cz79d

the other predefined expressions work, that looks like a bug on MSVC's part

I think these should be plain old string literals, but for the purpose or AST matchers and such, maybe we could have a "predefined macro" bit, or even an additional object that stores the name.

what do you mean by "an additional object that stores the name"?

I started trying to add a bit to StringLiteral that signifies if it's an MSVC predefined expression. being new to ast matchers, I'm having trouble figuring out how to make a matcher that matches both PredefinedExpr and some StringLiterals but also behaves like a VariadicDynCastAllOfMatcher.

something I noticed in one of the tests [1] is that there's a check

predefinedExpr(hasType(asString("const char[4]")),
                                     has(stringLiteral()))

thinking about the has(stringLiteral()), if somebody wants to get the literal for a predefined expr, presumably right now they're looking for a string literal child, but if we directly match a string literal then that would break? WDYT?

[1] https://github.com/llvm/llvm-project/blob/75ca15fcbb2e1b3671e41f971a000c6d59f5e5ae/clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp#L1064

[rsmith]

clang/lib/Sema/SemaExpr.cpp

3583–3588

I wonder if we could keep the representation as-is, but add an "is transparent" bit on PredefinedExpr that's set for string literal predefined expressions under -fms-extensions, and make IgnoreParens step over transparent PredefinedExprs. That seems like it'd exactly match the MS behavior, which seems to allow (eg) __func__ to appear anywhere a parenthesized string literal can appear.

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

3583–3588

That seems like a good approach to me. We've got spare bits in PredefinedExprBitfields, so this won't impact AST size, which is nice.

[aeubanks]

use IgnoreParens instead

[aeubanks]

add comment

clang/lib/Sema/SemaExpr.cpp

3583–3588

much much cleaner, thanks!

[aeubanks]

ping

[aeubanks]

ping

[aaron.ballman]

I think you're missing changes in ASTReaderStmt.cpp and ASTWriterStmt.cpp, so serialization through modules or PCH won't work without that.

clang/include/clang/AST/Expr.h

1995

I think we should keep PredefinedExpr general, so how about IsTransparent instead? Then we can add a comment above Create() that explains what IsTransparent does.

(This makes it easier for us to add predefined expressions that aren't string literals but are still transparently handled.)

[aeubanks]

address comments

[aeubanks]

In D146764#4310398, @aaron.ballman wrote:

I think you're missing changes in ASTReaderStmt.cpp and ASTWriterStmt.cpp, so serialization through modules or PCH won't work without that.

done. how would this sort of change be tested?

[aaron.ballman]

In D146764#4310655, @aeubanks wrote:

In D146764#4310398, @aaron.ballman wrote:

I think you're missing changes in ASTReaderStmt.cpp and ASTWriterStmt.cpp, so serialization through modules or PCH won't work without that.

done. how would this sort of change be tested?

Thanks! Typically with a test using modules or PCH, so something along these lines: https://github.com/llvm/llvm-project/blob/main/clang/test/Modules/cxx20-10-5-ex1.cpp and/or https://github.com/llvm/llvm-project/blob/main/clang/test/PCH/cxx-namespaces.cpp

[aeubanks]

add module test

[aaron.ballman]

clang/test/Modules/predefined.cpp

6 ↗

(On Diff #518777)

Er, should we have a -verify on this as well as // expected-no-diagnostics?

clang/test/Sema/ms_predefined_expr.cpp

6–10

Apologies for not noticing this earlier, but because this code isn't portable (for the standard predefined identifiers), I think we should also have a -pedantic test that ensures we get a diagnostic about accepting this code being a Microsoft extension.

I would recommend something along the lines of: initializing an array from a '%0' predefined identifier is a Microsoft extension put into a new warning group named -Wmicrosoft-init-from-predefined which is added to the -Wmicrosoft warning group.

[aeubanks]

add warning

[aeubanks]

added the warning, not sure if this needs more tests for testing the interaction between -pedantic, -Wmicrosoft, -Wmicrosoft-init-from-predefined or if that's already assumed to work

clang/test/Modules/predefined.cpp

6 ↗

(On Diff #518777)

isn't that tested by the Sema test?

[aaron.ballman]

In D146764#4313905, @aeubanks wrote:

added the warning, not sure if this needs more tests for testing the interaction between -pedantic, -Wmicrosoft, -Wmicrosoft-init-from-predefined or if that's already assumed to work

That's generally assumed to work, really the only question is whether we want Extension (only warned in -pedantic) or ExtWarn (warned by default). I think most of our Microsoft extension warnings are ExtWarn and it seems defensible for this to be on by default, but it'd be nice to know just how chatty this is on real world code bases too. I have the sneaking suspicion this code pattern doesn't come up often enough for this to be "too chatty", but if we find it triggers on popular libraries we might want to consider dialing it back to just Extension.

clang/test/Modules/predefined.cpp

6 ↗

(On Diff #518777)

Not quite. The Sema test does test a lot of the functionality, but this is testing whether we import the AST node properly and can still use it, so it's testing a different code path for generating the AST nodes used for doing semantic checks.

[aeubanks]

add -verify to module test

[aaron.ballman]

Precommit CI found some related failures that need to be addressed, but the functional changes are all looking good to me. Be sure to also add a release note for the fix as well.

[aeubanks]

only diagnose if we're initializing an array

[aaron.ballman]

Presuming that the issues found by precommit CI were resolved (current failures are unrelated to your changes), this LGTM but still needs a release note.

[aeubanks]

added a release note

[aeubanks]

the following crashes with this patch:

struct StringRef {
  StringRef(const char *);
};
template <typename T>
StringRef getTypeName() {
  StringRef s = __func__;
}

clang -cc1 -fms-extensions -std=c++17 -x c++ a.cpp -fsyntax-only

[rsmith]

Sorry for the late comments.

clang/lib/Sema/SemaInit.cpp

167–178

The duplication between this and IgnoreParensSingleStep is a code smell; can we expose IgnoreParensSingleStep and call it from here?

8511

We won't reach this case for const char arr[] = {__func__}; because in that case Args[0] will be an InitListExpr instead. Right now we accept this with no warning in MS extensions mode:

void f() {
  const char c[] = {__func__};
}

We should perform this check in the handling of SK_StringInit instead, in order to properly address that case.

Also, this will only catch the case where the argument is an unparenthesized predefined expression. You'll need to do something like repeatedly calling IgnoreParensSingleStep looking for a PredefinedExpr to cope with things like:

void g() {
  const char c[] = (__func__);
  const char d[] = _Generic(0, int: __func__);
}

clang/include/clang/AST/Expr.h

enum IdentKind {
     PrettyFunction,
     /// The same as PrettyFunction, except that the
     /// 'virtual' keyword is omitted for virtual member functions.
     PrettyFunctionNoVirtual
   };
 
 private:
   PredefinedExpr(SourceLocation L, QualType FNTy, IdentKind IK,
-                 StringLiteral *SL);
+                 bool IsTransparent, StringLiteral *SL);
 
   explicit PredefinedExpr(EmptyShell Empty, bool HasFunctionName);
 
   /// True if this PredefinedExpr has storage for a function name.
   bool hasFunctionName() const { return PredefinedExprBits.HasFunctionName; }
 
   void setFunctionName(StringLiteral *SL) {
     assert(hasFunctionName() &&
            "This PredefinedExpr has no storage for a function name!");
     *getTrailingObjects<Stmt *>() = SL;
   }
 
 public:
   /// Create a PredefinedExpr.
+  ///
+  /// If IsTransparent, the PredefinedExpr is transparently handled as a
+  /// StringLiteral.
   static PredefinedExpr *Create(const ASTContext &Ctx, SourceLocation L,
-                                QualType FNTy, IdentKind IK, StringLiteral *SL);
+                                QualType FNTy, IdentKind IK, bool IsTransparent,
+                                StringLiteral *SL);
 
   /// Create an empty PredefinedExpr.
   static PredefinedExpr *CreateEmpty(const ASTContext &Ctx,
                                      bool HasFunctionName);
 
   IdentKind getIdentKind() const {
     return static_cast<IdentKind>(PredefinedExprBits.Kind);
   }
 
+  bool isTransparent() const { return PredefinedExprBits.IsTransparent; }
+
   SourceLocation getLocation() const { return PredefinedExprBits.Loc; }
   void setLocation(SourceLocation L) { PredefinedExprBits.Loc = L; }
 
   StringLiteral *getFunctionName() {
     return hasFunctionName()
                ? static_cast<StringLiteral *>(*getTrailingObjects<Stmt *>())
                : nullptr;
   }

clang/include/clang/AST/IgnoreExpr.h

if (!GSE->isResultDependent())
       return GSE->getResultExpr();
   }
 
   else if (auto *CE = dyn_cast<ChooseExpr>(E)) {
     if (!CE->isConditionDependent())
       return CE->getChosenSubExpr();
   }
 
+  else if (auto *PE = dyn_cast<PredefinedExpr>(E)) {
+    if (PE->isTransparent())
+      return PE->getFunctionName();
+  }
+
   return E;
 }
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_IGNOREEXPR_H

clang/include/clang/AST/Stmt.h

class PredefinedExprBitfields {
     /// The kind of this PredefinedExpr. One of the enumeration values
     /// in PredefinedExpr::IdentKind.
     unsigned Kind : 4;
 
     /// True if this PredefinedExpr has a trailing "StringLiteral *"
     /// for the predefined identifier.
     unsigned HasFunctionName : 1;
 
+    /// True if this PredefinedExpr should be treated as a StringLiteral (for
+    /// MSVC compatibility).
+    unsigned IsTransparent : 1;
+
     /// The location of this PredefinedExpr.
     SourceLocation Loc;
   };
 
   class DeclRefExprBitfields {
     friend class ASTStmtReader; // deserialization
     friend class DeclRefExpr;
 

clang/lib/AST/ASTImporter.cpp

ExpectedStmt ASTNodeImporter::VisitPredefinedExpr(PredefinedExpr *E) {
   Error Err = Error::success();
   auto ToBeginLoc = importChecked(Err, E->getBeginLoc());
   auto ToType = importChecked(Err, E->getType());
   auto ToFunctionName = importChecked(Err, E->getFunctionName());
   if (Err)
     return std::move(Err);
 
   return PredefinedExpr::Create(Importer.getToContext(), ToBeginLoc, ToType,
-                                E->getIdentKind(), ToFunctionName);
+                                E->getIdentKind(), E->isTransparent(),
+                                ToFunctionName);
 }
 
 ExpectedStmt ASTNodeImporter::VisitDeclRefExpr(DeclRefExpr *E) {
 
   Error Err = Error::success();
   auto ToQualifierLoc = importChecked(Err, E->getQualifierLoc());
   auto ToTemplateKeywordLoc = importChecked(Err, E->getTemplateKeywordLoc());
   auto ToDecl = importChecked(Err, E->getDecl());

clang/lib/AST/Expr.cpp

std::string SYCLUniqueStableNameExpr::ComputeName(ASTContext &Context,
   Buffer.reserve(128);
   llvm::raw_string_ostream Out(Buffer);
   Ctx->mangleTypeName(Ty, Out);
 
   return Out.str();
 }
 
 PredefinedExpr::PredefinedExpr(SourceLocation L, QualType FNTy, IdentKind IK,
-                               StringLiteral *SL)
+                               bool IsTransparent, StringLiteral *SL)
     : Expr(PredefinedExprClass, FNTy, VK_LValue, OK_Ordinary) {
   PredefinedExprBits.Kind = IK;
   assert((getIdentKind() == IK) &&
          "IdentKind do not fit in PredefinedExprBitfields!");
   bool HasFunctionName = SL != nullptr;
   PredefinedExprBits.HasFunctionName = HasFunctionName;
+  PredefinedExprBits.IsTransparent = IsTransparent;
   PredefinedExprBits.Loc = L;
   if (HasFunctionName)
     setFunctionName(SL);
   setDependence(computeDependence(this));
 }
 
 PredefinedExpr::PredefinedExpr(EmptyShell Empty, bool HasFunctionName)
     : Expr(PredefinedExprClass, Empty) {
   PredefinedExprBits.HasFunctionName = HasFunctionName;
 }
 
 PredefinedExpr *PredefinedExpr::Create(const ASTContext &Ctx, SourceLocation L,
                                        QualType FNTy, IdentKind IK,
-                                       StringLiteral *SL) {
+                                       bool IsTransparent, StringLiteral *SL) {
   bool HasFunctionName = SL != nullptr;
   void *Mem = Ctx.Allocate(totalSizeToAlloc<Stmt *>(HasFunctionName),
                            alignof(PredefinedExpr));
-  return new (Mem) PredefinedExpr(L, FNTy, IK, SL);
+  return new (Mem) PredefinedExpr(L, FNTy, IK, IsTransparent, SL);
 }
 
 PredefinedExpr *PredefinedExpr::CreateEmpty(const ASTContext &Ctx,
                                             bool HasFunctionName) {
   void *Mem = Ctx.Allocate(totalSizeToAlloc<Stmt *>(HasFunctionName),
                            alignof(PredefinedExpr));
   return new (Mem) PredefinedExpr(EmptyShell(), HasFunctionName);
 }

clang/lib/Sema/SemaExpr.cpp

if (!currentDecl) {
     currentDecl = Context.getTranslationUnitDecl();
   }
 
   QualType ResTy;
   StringLiteral *SL = nullptr;
   if (cast<DeclContext>(currentDecl)->isDependentContext())
     ResTy = Context.DependentTy;
   else {
     // Pre-defined identifiers are of type char[x], where x is the length of
     // the string.
     auto Str = PredefinedExpr::ComputeName(IK, currentDecl);
     unsigned Length = Str.length();
 
     llvm::APInt LengthI(32, Length + 1);
     if (IK == PredefinedExpr::LFunction || IK == PredefinedExpr::LFuncSig) {
       ResTy =
           Context.adjustStringLiteralBaseType(Context.WideCharTy.withConst());
if (IK == PredefinedExpr::LFunction || IK == PredefinedExpr::LFuncSig) {
       ResTy = Context.getConstantArrayType(ResTy, LengthI, nullptr,
                                            ArrayType::Normal,
                                            /*IndexTypeQuals*/ 0);
       SL = StringLiteral::Create(Context, Str, StringLiteral::Ordinary,
                                  /*Pascal*/ false, ResTy, Loc);
     }
   }
 
-  return PredefinedExpr::Create(Context, Loc, ResTy, IK, SL);
+  return PredefinedExpr::Create(Context, Loc, ResTy, IK, LangOpts.MicrosoftExt,
+                                SL);
 }
 
 ExprResult Sema::BuildSYCLUniqueStableNameExpr(SourceLocation OpLoc,
                                                SourceLocation LParen,
                                                SourceLocation RParen,
                                                TypeSourceInfo *TSI) {
   return SYCLUniqueStableNameExpr::Create(Context, OpLoc, LParen, RParen, TSI);
 }
 
 ExprResult Sema::ActOnSYCLUniqueStableNameExpr(SourceLocation OpLoc,
                                                SourceLocation LParen,
                                                SourceLocation RParen,

clang/lib/Sema/SemaInit.cpp

 /// Update the type of a string literal, including any surrounding parentheses,
 /// to match the type of the object which it is initializing.
 static void updateStringLiteralType(Expr *E, QualType Ty) {
   while (true) {
     E->setType(Ty);
     E->setValueKind(VK_PRValue);
     if (isa<StringLiteral>(E) || isa<ObjCEncodeExpr>(E)) {
       break;
     } else if (ParenExpr *PE = dyn_cast<ParenExpr>(E)) {
       E = PE->getSubExpr();
     } else if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
       assert(UO->getOpcode() == UO_Extension);
       E = UO->getSubExpr();
     } else if (GenericSelectionExpr *GSE = dyn_cast<GenericSelectionExpr>(E)) {
       E = GSE->getResultExpr();
     } else if (ChooseExpr *CE = dyn_cast<ChooseExpr>(E)) {
       E = CE->getChosenSubExpr();
+    } else if (PredefinedExpr *PE = dyn_cast<PredefinedExpr>(E)) {
+      E = PE->getFunctionName();
     } else {
       llvm_unreachable("unexpected expr in string literal init");
     }
   }
 }
 
 /// Fix a compound literal initializing an array so it's correctly marked
 /// as an rvalue.
 static void updateGNUCompoundLiteralRValue(Expr *E) {
ExprResult InitializationSequence::Perform(Sema &S,
   }
 
   // OpenCL v2.0 s6.13.11.1. atomic variables can be initialized in global scope
   QualType ETy = Entity.getType();
   bool HasGlobalAS = ETy.hasAddressSpace() &&
                      ETy.getAddressSpace() == LangAS::opencl_global;
 
   if (S.getLangOpts().OpenCLVersion >= 200 &&
       ETy->isAtomicType() && !HasGlobalAS &&
       Entity.getKind() == InitializedEntity::EK_Variable && Args.size() > 0) {
     S.Diag(Args[0]->getBeginLoc(), diag::err_opencl_atomic_init)
         << 1
         << SourceRange(Entity.getDecl()->getBeginLoc(), Args[0]->getEndLoc());
     return ExprError();
   }
 
   QualType DestType = Entity.getType().getNonReferenceType();

clang/lib/Serialization/ASTReaderStmt.cpp

void ASTStmtReader::VisitSYCLUniqueStableNameExpr(SYCLUniqueStableNameExpr *E) {
   E->setTypeSourceInfo(Record.readTypeSourceInfo());
 }
 
 void ASTStmtReader::VisitPredefinedExpr(PredefinedExpr *E) {
   VisitExpr(E);
   bool HasFunctionName = Record.readInt();
   E->PredefinedExprBits.HasFunctionName = HasFunctionName;
   E->PredefinedExprBits.Kind = Record.readInt();
+  E->PredefinedExprBits.IsTransparent = Record.readInt();
   E->setLocation(readSourceLocation());
   if (HasFunctionName)
     E->setFunctionName(cast<StringLiteral>(Record.readSubExpr()));
 }
 
 void ASTStmtReader::VisitDeclRefExpr(DeclRefExpr *E) {
   VisitExpr(E);
 

clang/lib/Serialization/ASTWriterStmt.cpp

 }
 
 void ASTStmtWriter::VisitPredefinedExpr(PredefinedExpr *E) {
   VisitExpr(E);
 
   bool HasFunctionName = E->getFunctionName() != nullptr;
   Record.push_back(HasFunctionName);
   Record.push_back(E->getIdentKind()); // FIXME: stable encoding
+  Record.push_back(E->isTransparent());
   Record.AddSourceLocation(E->getLocation());
   if (HasFunctionName)
     Record.AddStmt(E->getFunctionName());
   Code = serialization::EXPR_PREDEFINED;
 }
 
 void ASTStmtWriter::VisitDeclRefExpr(DeclRefExpr *E) {
   VisitExpr(E);

clang/test/Sema/ms_predefined_expr.cpp

+// RUN: %clang_cc1 %s -fsyntax-only -Wmicrosoft -verify -fms-extensions
+// expected-no-diagnostics
+
+void f() {
+ const char a[] = __FUNCTION__;
+ const char b[] = __FUNCDNAME__;
+ const char c[] = __FUNCSIG__;
+ const char d[] = __func__;
+ const char e[] = __PRETTY_FUNCTION__;
+}