This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][CodeView] Use <lambda_n> as the display name for lambdas.
ClosedPublic

Authored by akhuang on Jan 21 2021, 5:08 PM.

Details

Summary

Currently (for codeview) lambdas have a string like <lambda_0> in
their mangled name, and don't have any display name. This change uses the
<lambda_0> as the display name, which helps distinguish between lambdas
in -gline-tables-only, since there are no linkage names there.
It also changes how we display lambda names; previously we used
<unnamed-tag>; now it will show <lambda_0>.

I added a function to the mangling context code to create this string;
for Itanium it just returns an empty string.

Bug: https://bugs.llvm.org/show_bug.cgi?id=48432

Diff Detail

Event Timeline

akhuang requested review of this revision.Jan 21 2021, 5:08 PM
akhuang created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 5:08 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added a comment.Jan 22 2021, 11:13 AM

I see a variety of presubmit failures in Harbormaster that seem related, so make sure to check those.

clang/include/clang/AST/Mangle.h
92

I think I would prefer to have this return a number. I think CGDebugInfo should be responsible for the formatting of the display names, not the mangler. Both the MS and Itanium manglers have private methods for numbering internal linkage lambdas, and I think it would be reasonable to hook those up to a public virtual method.

clang/lib/CodeGen/CGDebugInfo.cpp
320–321

I think this comment could be improved, and I think it would help address some of David's concerns in the previous patch about the format requirements of codeview.

clang/test/CodeGenCXX/debug-info-gline-tables-only-codeview.cpp
13

Please add tests for a lambda in an inline function and one in a non-inline function. The inline lambda will be externally visible and have a mangling number, and the non-inline one will be internal, and will not have a mangling number. They should both get <lambda_n> names, though.

akhuang updated this revision to Diff 318657.Jan 22 2021, 2:58 PM

-add to comment
-Add externally visible lambda to test case

akhuang updated this revision to Diff 319348.Jan 26 2021, 10:49 AM
akhuang marked an inline comment as done.

change to function returning a number, fill in ItaniumMangle function

rnk added a comment.Jan 26 2021, 11:29 AM

I see a potential issue, but I think this looks good otherwise.

clang/lib/AST/ItaniumMangle.cpp
210

This has the potential to create a situation where enabling debug info changes the internal mangled names that clang uses. To be *extra* safe, you could add a method like getAnonymousStructIdForDebugInfo that does .find instead of .insert, and use it here.

clang/lib/AST/MicrosoftMangle.cpp
236

This has a similar concern, this isn't a readonly operation.

akhuang added inline comments.Jan 26 2021, 12:22 PM
clang/include/clang/AST/Mangle.h
92

I used a string because we mangle some lambdas like <lambda_1_1> (when they're used as function parameters).
Maybe we don't need to differentiate between these lambdas though?

clang/lib/AST/ItaniumMangle.cpp
210

Oh, right, yeah. I guess I'll just let it return 0 if it's not found.

akhuang updated this revision to Diff 319379.Jan 26 2021, 12:23 PM

Avoid using getLambdaId function

rnk added inline comments.Jan 26 2021, 2:34 PM
clang/include/clang/AST/Mangle.h
92

I see. So, the situation we are worried about would look like:

struct Foo {
  static volatile int arg1_defaults, arg2_defaults;
  static void foo(
      int arg1 = []() { return ++arg1_defaults; }(),
      int arg2 = []() { return ++arg2_defaults; }());
};
void bar() { Foo::foo(); }

In this case, if we use numbers here, we'd end up using the names Foo::<lambda_1> Foo::<lambda_1>, and those would end up being duplicate LF_FUNC_IDs. Darn.

You were right, we better go back to using strings. Sorry about that. That makes a good test, though. :)

akhuang added inline comments.Jan 26 2021, 2:36 PM
clang/include/clang/AST/Mangle.h
92

Yep. I don't know what it should output for the Itanium version though. I guess it could just do the same thing, even though we don't use that string anywhere there.

akhuang updated this revision to Diff 319668.Jan 27 2021, 2:04 PM

make function return lambda string, and add test for lambda in function parameters

rnk accepted this revision.Jan 27 2021, 2:43 PM

lgtm, thanks!

This revision is now accepted and ready to land.Jan 27 2021, 2:43 PM
This revision was landed with ongoing or failed builds.Jan 28 2021, 4:31 PM
This revision was automatically updated to reflect the committed changes.
akhuang reopened this revision.Jan 28 2021, 6:42 PM
This revision is now accepted and ready to land.Jan 28 2021, 6:42 PM
akhuang updated this revision to Diff 320020.Jan 28 2021, 6:43 PM

Fix test / string code after test failures

This revision was landed with ongoing or failed builds.Jan 28 2021, 6:45 PM
This revision was automatically updated to reflect the committed changes.