This is an archive of the discontinued LLVM Phabricator instance.

[clang] Call printName to get name of Decl
ClosedPublic

Authored by dankm on Apr 26 2023, 10:23 AM.

Details

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

Diff Detail

Event Timeline

dankm created this revision.Apr 26 2023, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 10:23 AM
dankm requested review of this revision.Apr 26 2023, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 10:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
dankm added a comment.EditedApr 26 2023, 10:26 AM

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.

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 added inline comments.Apr 26 2023, 1:35 PM
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 added a comment.Apr 26 2023, 4:43 PM

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

dankm updated this revision to Diff 517426.Apr 26 2023, 6:53 PM

Add test case

dankm updated this revision to Diff 517427.Apr 26 2023, 6:54 PM

And restore original change

dankm added a comment.Apr 26 2023, 9:48 PM

Thanks. I didn't know that was created.

aaron.ballman added inline comments.Apr 27 2023, 6:43 AM
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 added inline comments.Apr 27 2023, 8:48 AM
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 added inline comments.Apr 27 2023, 9:03 AM
clang/test/CodeGen/debug-prefix-map.cpp
2

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

dankm updated this revision to Diff 517595.Apr 27 2023, 9:08 AM

Correct test case

dankm marked 2 inline comments as done.Apr 27 2023, 9:37 AM
aaron.ballman accepted this revision.Apr 28 2023, 10:51 AM

LGTM, but please add a release note when landing.

This revision is now accepted and ready to land.Apr 28 2023, 10:51 AM
dankm updated this revision to Diff 518050.Apr 28 2023, 2:06 PM

Rebase & add add release note.

dankm added a comment.Apr 28 2023, 2:07 PM

LGTM, but please add a release note when landing.

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

dankm edited the summary of this revision. (Show Details)May 2 2023, 8:26 AM
This revision was automatically updated to reflect the committed changes.