Add support of Windows Trace Logging macros

[RIscRIpt]

Summary

Consider the following code:

#include <windows.h>
#include <TraceLoggingActivity.h>
#include <TraceLoggingProvider.h>
#include <winmeta.h>

TRACELOGGING_DEFINE_PROVIDER(
    g_hMyComponentProvider,
    "SimpleTraceLoggingProvider",
    // {0205c616-cf97-5c11-9756-56a2cee02ca7}
    (0x0205c616,0xcf97,0x5c11,0x97,0x56,0x56,0xa2,0xce,0xe0,0x2c,0xa7));

void test()
{
    TraceLoggingFunction(g_hMyComponentProvider);
}

int main()
{
    TraceLoggingRegister(g_hMyComponentProvider);
    test();
    TraceLoggingUnregister(g_hMyComponentProvider);
}

It compiles with MSVC, but clang-cl reports an error:

C:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\shared/TraceLoggingActivity.h(377,30): note: expanded from macro '_tlgThisFunctionName'
#define _tlgThisFunctionName __FUNCTION__
                             ^
.\tl.cpp(14,5): error: cannot initialize an array element of type 'char' with an lvalue of type 'const char[5]'
    TraceLoggingFunction(g_hMyComponentProvider);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The second commit is not needed to support above code, however, during isolated tests in ms_predefined_expr.cpp
I found that MSVC accepts code with constexpr, whereas clang-cl does not.
I see that in most places PredefinedExpr is supported in constant evaluation, so I didn't wrap my code with `if(MicrosoftExt)`.

[cor3ntin]

clang/lib/Sema/Sema.cpp

1495–1499

I think this is reimplementing getCurFunctionOrMethodDecl
maybe we want to do

if(Decl* DC = getCurFunctionOrMethodDecl())
    return DC;
return dyn_cast_or_null<Decl>(CurrentContext);

Or something like that

[RIscRIpt]

clang/lib/Sema/Sema.cpp

1495–1499

Well, unfortunately, not really.

The previous implementation did call getCurFunctionOrMethodDecl(), but it returned nullptr when we were inside a RecordDecl.
If you examine the implementation of getFunctionLevelDeclContext(bool AllowLambda), it halts if isa<RecordDecl>(DC). I tried patching getFunctionLevelDeclContext() to skip RecordDecl, but this triggered around 70 clang/tests. I didn't want to delve into understanding the failure of each test. If you believe there's an issue with our current code, I can allocate time to investigate each specific test case.

[cor3ntin]

clang/lib/Sema/Sema.cpp

1495–1499

You are right, i missed that.
I wish we had a better name for this function but I can't think of anything

1496–1497

[RIscRIpt]

clang/lib/Sema/Sema.cpp

1495–1499

A perfect name would be getFunctionLevelDeclContext, but it's taken. Welp... I'll try to dig-into related functions, and update when I find something. An alternative solution would be to parameterize (in some way) getFunctionLevelDeclContext (e.g. add bool flags, or list of skippable types, etc.).

[RIscRIpt]

Rebase onto main, address review comments

clang/lib/Sema/Sema.cpp

1495–1499

A perfect name would be getFunctionLevelDeclContext, but it's taken.

No, it's not. I found a better one, which was hiding in a plain sight.

Regarding using a common implementation: with a new function name it's clear that we cannot do that - they serve different purposes. For example, this function may return BlockDecl or CapturedDecl (which is accepted by PredefinedExpr::ComputeName) whereas getFunctionLevelDeclContext cannot.

[RIscRIpt]

Use isa<...> in getPredefinedExprDeclContext

[RIscRIpt]

I missed this comment

[RIscRIpt]

The current implementation of getPredefinedExprDeclContext could be easily moved to a static function inside clang/lib/Sema/SemaExpr.cpp as it depends only on Sema::CurContext WDYT?

[cor3ntin]

Looks mostly good to me, thanks!

In D158591#4613868, @RIscRIpt wrote:

The current implementation of getPredefinedExprDeclContext could be easily moved to a static function inside clang/lib/Sema/SemaExpr.cpp as it depends only on Sema::CurContext WDYT?

With the new name, that would make sense. we could keep close to where it is used.

clang/include/clang/Sema/Sema.h

3617–3620 ↗

(On Diff #553123)

clang/lib/AST/ExprConstant.cpp

7527–7529

I wonder if we should modify the new interpreter at the same time to avoid giving more work to @tbaeder.
You can model it on ByteCodeExprGen<Emitter>::VisitSubstNonTypeTemplateParmExpr

clang/lib/Sema/Sema.cpp

1495–1499

Yeah, until we find another use case for this thing, this looks reasonable.

I also thought of getAnyFunctionContext or something along those lines but.. meh. what you have is fine, thanks!

[RIscRIpt]

Address review comments

[RIscRIpt]

Regarding new interpreter.

It takes more time and effort than I expected.
Here's what I found:

1. Assertion in ByteCodeExprGen<Emitter>::VisitPredefinedExpr seems unnecessary.
2. StringLiteral of PredefinedExpr gets initialized in ByteCodeExprGen<Emitter>::VisitStringLiteral; I observe creation of InitMap, and initialization of each element of array.
3. However when I am at the following call-stack (just before exiting constexpr interpreter and doing final checks):

00 00000066`04580990 00007ff6`55fb7257     clang!clang::interp::CheckArrayInitialized+0x257 [C:\projects\my-llvm\clang\lib\AST\Interp\Interp.cpp @ 425] 
01 00000066`04580b50 00007ff6`55f67ae6     clang!clang::interp::CheckCtorCall+0xc7 [C:\projects\my-llvm\clang\lib\AST\Interp\Interp.cpp @ 476] 
02 00000066`04580bb0 00007ff6`55ebdd17     clang!clang::interp::CheckGlobalCtor+0x36 [C:\projects\my-llvm\clang\lib\AST\Interp\Interp.h @ 1681] 
03 00000066`04580bf0 00007ff6`55fb3e3d     clang!clang::interp::EvalEmitter::emitCheckGlobalCtor+0x57 [C:\projects\my-llvm\build\tools\clang\lib\AST\Opcodes.inc @ 4133] 
04 00000066`04580c20 00007ff6`55fb4db1     clang!clang::interp::ByteCodeExprGen<clang::interp::EvalEmitter>::visitGlobalInitializer+0xdd [C:\projects\my-llvm\clang\lib\AST\Interp\ByteCodeExprGen.h @ 180] 
05 00000066`04580c90 00007ff6`55fb33ce     clang!clang::interp::ByteCodeExprGen<clang::interp::EvalEmitter>::visitVarDecl+0x291 [C:\projects\my-llvm\clang\lib\AST\Interp\ByteCodeExprGen.cpp @ 1827] 
06 00000066`04580e40 00007ff6`55eb7023     clang!clang::interp::ByteCodeExprGen<clang::interp::EvalEmitter>::visitDecl+0x8e [C:\projects\my-llvm\clang\lib\AST\Interp\ByteCodeExprGen.cpp @ 1772] 
07 00000066`04580f30 00007ff6`55d18d54     clang!clang::interp::EvalEmitter::interpretDecl+0x33 [C:\projects\my-llvm\clang\lib\AST\Interp\EvalEmitter.cpp @ 39] 
08 00000066`04580f90 00007ff6`55633abf     clang!clang::interp::Context::evaluateAsInitializer+0xf4 [C:\projects\my-llvm\clang\lib\AST\Interp\Context.cpp @ 74] 
09 00000066`04581290 00007ff6`553b4398     clang!clang::Expr::EvaluateAsInitializer+0x28f [C:\projects\my-llvm\clang\lib\AST\ExprConstant.cpp @ 15545] 
0a 00000066`04581880 00007ff6`553b4744     clang!clang::VarDecl::evaluateValueImpl+0x148 [C:\projects\my-llvm\clang\lib\AST\Decl.cpp @ 2555] 
0b 00000066`04581940 00007ff6`537e94fc     clang!clang::VarDecl::checkForConstantInitialization+0xe4 [C:\projects\my-llvm\clang\lib\AST\Decl.cpp @ 2625] 
0c 00000066`04581980 00007ff6`537f8109     clang!clang::Sema::CheckCompleteVariableDeclaration+0xddc [C:\projects\my-llvm\clang\lib\Sema\SemaDecl.cpp @ 14315] 
0d 00000066`04582770 00000000`00000000     clang!clang::Sema::AddInitializerToDecl+0x2669 [C:\projects\my-llvm\clang\lib\Sema\SemaDecl.cpp @ 13712]

In CheckArrayInitialized BasePtr.atIndex(I).isInitialized() returns false, because InitMap does not exist. And it seems that BasePtr points to a different object, not the one which corresponds to StringLiteral (at step 2). This new interpreter does not use clang's AST, so it becomes difficult to understand what's happening. Any help would be appreciated. cc @tbaeder.

[cor3ntin]

I'm happy punting the new interpreter to a subsequent PR, maybe there are unresolved issues with string literals?
Anyway, we don't have to scope creep further, LGTM

Thanks for working on this!

[RIscRIpt]

In D158591#4616728, @RIscRIpt wrote:

And it seems that BasePtr points to a different object, not the one which corresponds to StringLiteral (at step 2).

If I understand everything correctly, then in visitGlobalInitializer, when we return from visitInitializer our this->Locals becomes empty. So this->emitCheckGlobalCtor(Init) fails, because it sees are elements are uninitialized.

Thanks for approval! Let me know if I should report my observations about new constexpr interpreter somewhere else.

[RIscRIpt]

Rebased onto main, fixed commit messages, fixed formatting, re-run lit for clang/test

[cor3ntin]

@RIscRIpt Do you need further help on this patch? You have commit access, right?
(I'm trying to make sure things don't fall through the cracks as we move to github)

[RIscRIpt]

@cor3ntin, thanks for asking. No, I do not have access. Please merge. Commits should have correct email and name.

[cor3ntin]

Oh! I should have asked sooner. Will do! Thanks

[cor3ntin]

Hum. before I do

• I think the commit message could be more explicit as to what the patch does ("Add support of Windows Trace Logging macros" is a bit vague)
• Do we need a release note ?

[RIscRIpt]

In D158591#4636814, @cor3ntin wrote:

Hum. before I do

• I think the commit message could be more explicit as to what the patch does ("Add support of Windows Trace Logging macros" is a bit vague)
• Do we need a release note ?

I agree that single commit message is vague. That's why I submitted two separate commits with detailed messages (the first one has some details in body).

I don't think we need release notes, because it's a bug fix.

P.S. I don't have access to PC until next week, I am okay if you adjust commit messages in a way you want.

[cor3ntin]

Add release note

[cor3ntin]

@RIscRIpt I added release notes, let me know if you are happy with everything before i land

[RIscRIpt]

Looks good. Thanks!

clang/lib/AST/ExprConstant.cpp

public:
 
   bool VisitStmt(const Stmt *) {
     llvm_unreachable("Expression evaluator should not be called on stmts");
   }
   bool VisitExpr(const Expr *E) {
     return Error(E);
   }
 
+  bool VisitPredefinedExpr(const PredefinedExpr *E) {
+    return StmtVisitorTy::Visit(E->getFunctionName());
+  }
   bool VisitConstantExpr(const ConstantExpr *E) {
     if (E->hasAPValueResult())
       return DerivedSuccess(E->getAPValueResult(), E);
 
     return StmtVisitorTy::Visit(E->getSubExpr());
   }
 
   bool VisitParenExpr(const ParenExpr *E)

clang/lib/Sema/Sema.cpp

 NamedDecl *Sema::getCurFunctionOrMethodDecl() const {
   DeclContext *DC = getFunctionLevelDeclContext();
   if (isa<ObjCMethodDecl>(DC) || isa<FunctionDecl>(DC))
     return cast<NamedDecl>(DC);
   return nullptr;
 }
 
 Decl *Sema::getCurLocalScopeDecl() {
-  if (const BlockScopeInfo *BSI = getCurBlock())
-    return BSI->TheDecl;
-  if (const LambdaScopeInfo *LSI = getCurLambda())
-    return LSI->CallOperator;
-  if (const CapturedRegionScopeInfo *CSI = getCurCapturedRegion())
-    return CSI->TheCapturedDecl;
-  if (NamedDecl *ND = getCurFunctionOrMethodDecl())
-    return ND;
-  return nullptr;
+  DeclContext *DC = CurContext;
+  while (DC && !isa<BlockDecl>(DC) && !isa<CapturedDecl>(DC) &&
+         !isa<FunctionDecl>(DC) && !isa<ObjCMethodDecl>(DC))
+    DC = DC->getParent();
+  return dyn_cast_or_null<Decl>(DC);
 }
 
 LangAS Sema::getDefaultCXXMethodAddrSpace() const {
   if (getLangOpts().OpenCL)
     return getASTContext().getDefaultOpenCLPointeeAddrSpace();
   return LangAS::Default;
 }
 

clang/test/Sema/ms_predefined_expr.cpp

 
 void test_static_assert() {
   static_assert(true, __FUNCTION__); // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
 }
 
 void test_char_injection(decltype(sizeof('"')), decltype(sizeof("()"))) {
   unused("" __FUNCSIG__); // expected-warning{{expansion of predefined identifier '__FUNCSIG__' to a string literal is a Microsoft extension}}
 }
+
+void test_in_struct_init() {
+  struct {
+    char F[sizeof(__FUNCTION__)];
+  } s1 = { __FUNCTION__ }; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+
+  struct {
+    char F[sizeof("F:" __FUNCTION__)]; // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+  } s2 = { "F:" __FUNCTION__ }; // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+
+  class C {
+    public:
+      struct {
+        char F[sizeof(__FUNCTION__)];
+      } s;
+  } c1 = { { __FUNCTION__ } }; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+}
+
+void test_in_constexpr_struct_init() {
+  struct {
+    char F[sizeof(__FUNCTION__)];
+  } constexpr s1 = { __FUNCTION__ }; // expected-warning{{initializing an array from a '__FUNCTION__' predefined identifier is a Microsoft extension}}
+  ASSERT_EQ(__FUNCTION__, s1.F);
+
+  struct {
+    char F[sizeof("F:" __FUNCTION__)]; // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+  } constexpr s2 = { "F:" __FUNCTION__ }; // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+  ASSERT_EQ("F:" __FUNCTION__, s2.F); // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+
+  class C {
+    public:
+      struct {
+        char F[sizeof("F:" __FUNCTION__)]; // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+      } s;
+  } constexpr c1 = { { "F:" __FUNCTION__ } }; // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+  ASSERT_EQ("F:" __FUNCTION__, c1.s.F); // expected-warning{{expansion of predefined identifier '__FUNCTION__' to a string literal is a Microsoft extension}}
+}