[clang-cl] Fix value of FUNCTION and FUNC in MSVC mode for c++. #66120

zahiraam · 2023-09-12T18:27:10Z

Predefined macro FUNCTION in clang is not returning the same string than MS for templated functions.

See https://godbolt.org/z/q3EKn5zq4

For the same test case MSVC is returning:

function: TestClass::TestClass
function: TestStruct::TestStruct
function: TestEnum::TestEnum

llvmbot · 2023-09-12T18:28:26Z

@llvm/pr-subscribers-clang

Changes

None

Full diff: https://github.com/llvm/llvm-project/pull/66120.diff

2 Files Affected:

(modified) clang/lib/AST/Expr.cpp (+20)
(modified) clang/lib/AST/TypePrinter.cpp (+4)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 4f3837371b3fc5e..55b6e2968487b86 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -727,6 +727,26 @@ StringRef PredefinedExpr::getIdentKindName(PredefinedExpr::IdentKind IK) {
 std::string PredefinedExpr::ComputeName(IdentKind IK, const Decl *CurrentDecl) {
   ASTContext &Context = CurrentDecl->getASTContext();
 
+  if (CurrentDecl->getASTContext().getTargetInfo().getCXXABI().isMicrosoft() &&
+      IK == PredefinedExpr::Function) {
+    if (const FunctionDecl *FD = dyn_cast(CurrentDecl)) {
+      SmallString<256> Name;
+      llvm::raw_svector_ostream Out(Name);
+      PrintingPolicy Policy(Context.getLangOpts());
+      Policy.AlwaysIncludeTypeForTemplateArgument = true;
+      std::string Proto;
+      llvm::raw_string_ostream POut(Proto);
+      const FunctionDecl *Decl = FD;
+      if (const FunctionDecl *Pattern = FD->getTemplateInstantiationPattern())
+        Decl = Pattern;
+      const FunctionType *AFT = Decl->getType()->getAs();
+      const FunctionProtoType *FT = nullptr;
+      if (FD->hasWrittenPrototype())
+        FT = dyn_cast(AFT);
+      FD->printQualifiedName(POut, Policy);
+      return std::string(POut.str());
+    }
+  }
   if (IK == PredefinedExpr::FuncDName) {
     if (const NamedDecl *ND = dyn_cast(CurrentDecl)) {
       std::unique_ptr MC;
diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index eb69d0bb8755b48..676ce166312adf4 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -2218,6 +2218,10 @@ printTo(raw_ostream &OS, ArrayRef Args, const PrintingPolicy &Policy,
     } else {
       if (!FirstArg)
         OS << Comma;
+      // zahira
+      //if (Argument.getKind() == TemplateArgument::Type)
+      //  OS << "class ";
+
       // Tries to print the argument with location info if exists.
       printArgument(Arg, Policy, ArgOS,
                     TemplateParameterList::shouldIncludeTypeForArgument(

zahiraam · 2023-09-15T14:24:29Z

clang/lib/AST/TypePrinter.cpp

@@ -2218,6 +2218,9 @@ printTo(raw_ostream &OS, ArrayRef<TA> Args, const PrintingPolicy &Policy,
    } else {
      if (!FirstArg)
        OS << Comma;
+      //if (Argument.getKind() == TemplateArgument::Type)
+      //  OS << "class ";
+


TODO: if we go with this solution, this needs to happen here, but it should be under some conditions?

I would move it into printArgument. If you have to, it would be find to add a new flag to PrintingPolicy. We have lots of fine-grained flags there.

zahiraam · 2023-09-19T19:15:27Z

There are still some debug info LIT tests that are failing, but I am not going to edit them because I am not quite sure that's correct. Probably adding the "class" keyword shouldn't happen at all times even when the MSVCPolicy is true?

rnk

I wanted to make some minor naming suggestions, and I used the GitHub "edit this file" feature for the first time to commit them as new commits to your branch. I've never done this before, so I don't really know how it works, but clang-format isn't available, so now I've messed up the formatting...

I think you should be able to pull the suggested changes down, reformat the affected lines, commit that, push an updated commit, and then I can stamp that? I apologize if this isn't straightforward, it's my first time trying the tools.

zahiraam · 2023-09-21T18:58:26Z

I wanted to make some minor naming suggestions, and I used the GitHub "edit this file" feature for the first time to commit them as new commits to your branch. I've never done this before, so I don't really know how it works, but clang-format isn't available, so now I've messed up the formatting...

I think you should be able to pull the suggested changes down, reformat the affected lines, commit that, push an updated commit, and then I can stamp that? I apologize if this isn't straightforward, it's my first time trying the tools.

No worries. I think it is synced up now. Thanks.

rnk

Thanks!

RIscRIpt · 2023-09-23T13:37:26Z

This implementation has bugs, see #66114 (comment)

zahiraam · 2023-09-25T12:57:05Z

This implementation has bugs, see #66114 (comment)

@RIscRIpt Thanks. Will look at them.

Caslyn · 2023-09-27T16:25:40Z

Hi @zahiraam - can we revert this change until #66114 (comment) is resolved? Unfortunately this is impacting our downstream windows-x64 builder.

zahiraam · 2023-09-27T16:58:52Z

Hi @zahiraam - can we revert this change until #66114 (comment) is resolved? Unfortunately this is impacting our downstream windows-x64 builder.

I have a fix for it. It should be ready by eod (for me) today. But if that can't wait, sure we can revert it.

petrhosek · 2023-09-27T17:35:40Z

Our preference would be to revert the change and reland it later since this doesn't appear to be a quick fix, which is in line with https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

zahiraam · 2023-09-27T17:59:33Z

Our preference would be to revert the change and reland it later since this doesn't appear to be a quick fix, which is in line with https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

No problem. Go ahead and revert it. Thanks.

…e for c++. (#66120)" This reverts commit 265568c. See #66114 (comment)

AaronBallman · 2023-09-27T18:30:16Z

I've reverted the changes in 9243b1b, but you should handle the revert yourself next time because you have commit privileges and it's possible the person requesting a revert does not. (If you can't do the revert immediately for whatever reason, it's fine to tell people something along the lines of "I'll revert by unless someone else is able to revert it sooner.")

zahiraam · 2023-09-27T18:34:37Z

I've reverted the changes in 9243b1b, but you should handle the revert yourself next time because you have commit privileges and it's possible the person requesting a revert does not. (If you can't do the revert immediately for whatever reason, it's fine to tell people something along the lines of "I'll revert by unless someone else is able to revert it sooner.")

Sorry. Didn't realize that (I would have expected that the person would let me know). Thanks.

AaronBallman · 2023-09-27T18:40:30Z

I've reverted the changes in 9243b1b, but you should handle the revert yourself next time because you have commit privileges and it's possible the person requesting a revert does not. (If you can't do the revert immediately for whatever reason, it's fine to tell people something along the lines of "I'll revert by unless someone else is able to revert it sooner.")

Sorry. Didn't realize that (I would have expected that the person would let me know). Thanks.

No worries! We could probably make the policy documentation more clear about that.

…e for c++. (llvm#66120)" This reverts commit 265568c. See llvm#66114 (comment)

Predefined macro FUNCTION in clang is not returning the same string than MS for templated functions. See https://godbolt.org/z/q3EKn5zq4 For the same test case MSVC is returning: function: TestClass::TestClass function: TestStruct::TestStruct function: TestEnum::TestEnum The initial work for this was in the reverted patch (#66120). This patch solves the issues raised in the reverted patch.

Predefined macro FUNCTION in clang is not returning the same string than MS for templated functions. See https://godbolt.org/z/q3EKn5zq4 For the same test case MSVC is returning: function: TestClass::TestClass function: TestStruct::TestStruct function: TestEnum::TestEnum The initial work for this was in the reverted patch (llvm#66120). This patch solves the issues raised in the reverted patch.

[clang-cl] Fix for __FUNCTION__ in c++.

3fcfa30

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 12, 2023

zahiraam added 2 commits September 13, 2023 06:45

Testing.

f2c2d9c

Merge branch 'llvm:main' into FixFunction

0ebc0d6

zahiraam commented Sep 15, 2023

View reviewed changes

zahiraam mentioned this pull request Sep 15, 2023

[clang-cl] __FUNCTION__ used in templated function is not returning the same string than MSVC. #66114

Open

zahiraam added 2 commits September 18, 2023 07:53

Merge branch 'llvm:main' into FixFunction

191e30c

Addressed review comments.

5e85929

zahiraam requested review from cor3ntin, tahonermann and rnk September 19, 2023 21:30

rnk requested review from zmodem and removed request for rnk September 20, 2023 20:39

Addressed review comments and fixed some LIT tests.

1364bb8

zahiraam marked this pull request as ready for review September 21, 2023 15:40

llvmbot added the clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html label Sep 21, 2023

rnk added 3 commits September 21, 2023 10:09

Update PrettyPrinter.h

c51bf3f

Update Expr.cpp

7aacf60

Update TypePrinter.cpp

8d83fa3

rnk reviewed Sep 21, 2023

View reviewed changes

zahiraam added 2 commits September 21, 2023 11:36

Added changes suggested by rnk

4721f5d

sync commits

d9800c5

rnk approved these changes Sep 21, 2023

View reviewed changes

zahiraam changed the title ~~[clang-cl] Fix for __FUNCTION__ in c++.~~ [clang-cl] Fix value of __FUNCTION__ and __FUNC__ in MSVC mode for c++. Sep 22, 2023

zahiraam merged commit 265568c into llvm:main Sep 22, 2023
2 checks passed

kstoimenov mentioned this pull request Sep 22, 2023

Add memcpm test kstoimenov/llvm-project#13

Closed

AaronBallman added a commit that referenced this pull request Sep 27, 2023

Revert "[clang-cl] Fix value of __FUNCTION__ and __FUNC__ in MSVC mod…

9243b1b

…e for c++. (#66120)" This reverts commit 265568c. See #66114 (comment)

legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023

Revert "[clang-cl] Fix value of __FUNCTION__ and __FUNC__ in MSVC mod…

621b5ae

…e for c++. (llvm#66120)" This reverts commit 265568c. See llvm#66114 (comment)

Sirraide mentioned this pull request Mar 5, 2024

[clang-cl] Fix value of __FUNCTION__ in MSVC mode. #84014

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-cl] Fix value of FUNCTION and FUNC in MSVC mode for c++. #66120

[clang-cl] Fix value of FUNCTION and FUNC in MSVC mode for c++. #66120

zahiraam commented Sep 12, 2023 •

edited

llvmbot commented Sep 12, 2023

None

zahiraam Sep 15, 2023

rnk Sep 20, 2023

zahiraam commented Sep 19, 2023

rnk left a comment

zahiraam commented Sep 21, 2023

rnk left a comment

RIscRIpt commented Sep 23, 2023

zahiraam commented Sep 25, 2023

Caslyn commented Sep 27, 2023 •

edited

zahiraam commented Sep 27, 2023

petrhosek commented Sep 27, 2023

zahiraam commented Sep 27, 2023

AaronBallman commented Sep 27, 2023

zahiraam commented Sep 27, 2023

AaronBallman commented Sep 27, 2023

[clang-cl] Fix value of __FUNCTION__ and __FUNC__ in MSVC mode for c++. #66120

[clang-cl] Fix value of __FUNCTION__ and __FUNC__ in MSVC mode for c++. #66120

Conversation

zahiraam commented Sep 12, 2023 • edited

llvmbot commented Sep 12, 2023

None

zahiraam Sep 15, 2023

Choose a reason for hiding this comment

rnk Sep 20, 2023

Choose a reason for hiding this comment

zahiraam commented Sep 19, 2023

rnk left a comment

Choose a reason for hiding this comment

zahiraam commented Sep 21, 2023

rnk left a comment

Choose a reason for hiding this comment

RIscRIpt commented Sep 23, 2023

zahiraam commented Sep 25, 2023

Caslyn commented Sep 27, 2023 • edited

zahiraam commented Sep 27, 2023

petrhosek commented Sep 27, 2023

zahiraam commented Sep 27, 2023

AaronBallman commented Sep 27, 2023

zahiraam commented Sep 27, 2023

AaronBallman commented Sep 27, 2023

[clang-cl] Fix value of FUNCTION and FUNC in MSVC mode for c++. #66120

[clang-cl] Fix value of FUNCTION and FUNC in MSVC mode for c++. #66120

zahiraam commented Sep 12, 2023 •

edited

Caslyn commented Sep 27, 2023 •

edited