[clang] Call printName to get name of Decl

[dankm]

Summary

Rather than sending a name directly to the stream, use printName
to preserve any PrintingPolicy. This ensures that names are properly
affected by path remapping.

Fixes: https://github.com/llvm/llvm-project/issues/62192

[dankm]

This fixes an issue I wasn't able to reduce. Some lambdas wound up with the real filename in their name, rather than the remapped path from -ffile-prefix-map.

[aaron.ballman]

Thank you for working on this! Please be sure to add a release note and test coverage for the changes.

clang/lib/AST/DeclarationName.cpp

119–125

This isn't the right way to go about this -- the diagnostic engine knows how to print a NamedDecl when given one, and that's what should be fixed. (We pass in a NamedDecl all over the place -- it's preferred to passing in a string.)

https://github.com/llvm/llvm-project/blob/b893368fd4fdf40b7778df8d0b17312def1a8156/clang/lib/AST/ASTDiagnostic.cpp#L460 is one place where that happens, and https://github.com/llvm/llvm-project/blob/b893368fd4fdf40b7778df8d0b17312def1a8156/clang/lib/Basic/Diagnostic.cpp#L1060 is another, so I'd start investigating from there.

[dankm]

clang/lib/AST/DeclarationName.cpp

119–125

Maybe I could have been more clear in my description of the problem; it's not diagnostics. It's naming in the output. With -ffile-prefix-map in use and without this change I see this:

% strings lib/x86_64-dankm-freebsd13.2/libc++.so.1.0|grep barrier.cpp
std::__1::__barrier_algorithm_base::__state_t::(unnamed struct at /home/dan/llvm/llvm-wip/libcxx/src/barrier.cpp:24:9)

when I expect (and get with this change):

% strings lib/x86_64-dankm-freebsd13.2/libc++.so.1.0|grep barrier.cpp
std::__1::__barrier_algorithm_base::__state_t::(unnamed struct at /llvm-root/libcxx/src/barrier.cpp:24:9)

It looks like every other call in the DeclarationName::print function preserves the policy, these weren't. As far as I can tell, this is the only place this is used.

[dankm]

Hoorat for me. I just managed to produce a reduced test case. That'll be added to the review.

[dankm]

Add test case

[dankm]

And restore original change

[raj.khem]

This would fix https://github.com/llvm/llvm-project/issues/62192

[dankm]

In D149272#4301103, @raj.khem wrote:

This would fix https://github.com/llvm/llvm-project/issues/62192

Thanks. I didn't know that was created.

[aaron.ballman]

clang/lib/AST/DeclarationName.cpp

119–125

Ah, thank you for the explanation, now I see what's going on.

clang/test/CodeGen/debug-prefix-map.cpp

2

I'm taking a guess at correcting the RUN line here, but more tweaks may be needed. Basically, %clang++ isn't a thing, so this test fails. Most of our tests should be testing %clang_cc1 to test the frontend invocation, but some of the debug prefix map tests use %clang to test the driver functionality. I don't think there's a need to test the driver here, so I went with %clang_cc1. I'm not 100% certain whether -debug-info-kind= is necessary or not, but the other tests seem to be using that, which may be worth paying attention to.

Finally, there's no need to have a custom check prefix for FileCheck, the builtin CHECK prefix suffices.

12

[dankm]

clang/test/CodeGen/debug-prefix-map.cpp

2

Okay, noted about clang++, it did work for me locally, but that doesn't mean it's correct. Good point about only testing the frontend action, the driver test is unnecessary.

The custom check prefix is just a cut & paste artefact from copying from the C version of the debug-prefix-map test, originally I was going to update that test to a C++ file but opted later for just creating a new test.

[dankm]

clang/test/CodeGen/debug-prefix-map.cpp

2

Ah, I see now it failed on the windows build in my test. Neat.

[dankm]

Correct test case

[aaron.ballman]

LGTM, but please add a release note when landing.

[dankm]

Rebase & add add release note.

[dankm]

In D149272#4306103, @aaron.ballman wrote:

LGTM, but please add a release note when landing.

Thanks, I do not have permission to land, are you able to?

clang/lib/AST/Decl.cpp

case Module::PrivateModuleFragment:
     // The private module fragment is part of its containing module for linkage
     // purposes.
     return M->Parent;
   }
 
   llvm_unreachable("unknown module kind");
 }
 
-void NamedDecl::printName(raw_ostream &OS, const PrintingPolicy&) const {
-  OS << Name;
+void NamedDecl::printName(raw_ostream &OS, const PrintingPolicy &Policy) const {
+  Name.print(OS, Policy);
 }
 
 void NamedDecl::printName(raw_ostream &OS) const {
   printName(OS, getASTContext().getPrintingPolicy());
 }
 
 std::string NamedDecl::getQualifiedNameAsString() const {
   std::string QualName;

clang/lib/AST/DeclarationName.cpp

 }
 
 static void printCXXConstructorDestructorName(QualType ClassType,
                                               raw_ostream &OS,
                                               PrintingPolicy Policy) {
   // We know we're printing C++ here. Ensure we print types properly.
   Policy.adjustForCPlusPlus();
 
   if (const RecordType *ClassRec = ClassType->getAs<RecordType>()) {
-    OS << *ClassRec->getDecl();
+    ClassRec->getDecl()->printName(OS, Policy);
     return;
   }
   if (Policy.SuppressTemplateArgsInCXXConstructors) {
     if (auto *InjTy = ClassType->getAs<InjectedClassNameType>()) {
-      OS << *InjTy->getDecl();
+      InjTy->getDecl()->printName(OS, Policy);
       return;
     }
   }
   ClassType.print(OS, Policy);
 }
 
 void DeclarationName::print(raw_ostream &OS,
                             const PrintingPolicy &Policy) const {

clang/test/CodeGen/debug-prefix-map.cpp

+// RUN: %clang_cc1 -debug-info-kind=standalone -fdebug-prefix-map=%p=./UNLIKELY_PATH/empty -S %s -emit-llvm -o - | FileCheck %s
+
+struct alignas(64) an {
+  struct {
+    unsigned char x{0};
+  } arr[64];
+};
+
+struct an *pan = new an;
+
+// CHECK: !DISubprogram(name: "(unnamed struct at ./UNLIKELY_PATH/empty{{/|\\\\}}{{.*}}",