Page MenuHomePhabricator

[AST] Mangle LambdaContextDecl for top level decl
Needs ReviewPublic

Authored by zequanwu on Mon, May 18, 12:51 PM.

Details

Summary

Bug filed here: https://bugs.llvm.org/show_bug.cgi?id=45213

To resolve it, we let the checks for mangling LambdaContextDecl to be analogous to ItaniumMangle strategy: https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ItaniumMangle.cpp#L1829

Diff Detail

Event Timeline

zequanwu created this revision.Mon, May 18, 12:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 18, 12:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rnk added inline comments.Mon, May 18, 5:57 PM
clang/lib/AST/MicrosoftMangle.cpp
949–952

Will this fix work if the inline variable is in a namespace, or inline function?

This code seems like maybe it should live in getEffectiveDeclContext. Are you sure the fix won't be there?

zequanwu marked an inline comment as done.Tue, May 19, 1:01 PM
zequanwu added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
949–952

Because whenever we got a LambdaExpr and it is inside VarDecl, we want to add @[varaible] after @<lambda_1>. So, the demangled result will have [variable]::<lambda_1>.

I think we should remove the check for record and translation unit.

zequanwu marked an inline comment as done.Tue, May 19, 1:03 PM
zequanwu added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
949–952

Otherwise, the fix won't work if inline variable is inside namespace.

zequanwu updated this revision to Diff 265031.Tue, May 19, 2:35 PM

Remove check for context of LambdaContextDecl.
Update test cases.

zequanwu updated this revision to Diff 265046.Tue, May 19, 2:35 PM

Update test cases.

rnk added a comment.Wed, May 20, 4:10 PM

After looking at the code more, I'm more convinced that your fix is in the right place.

clang/lib/AST/MicrosoftMangle.cpp
955

I think this code is supposed to be analogous to the ItaniumMangle.cpp code here:
https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/ItaniumMangle.cpp#L1829

Can you add in && !isa<ParmVarDecl>(LambdaContextDecl) to this condition? I believe it will prevent changing the mangling in some of the test cases you have.

clang/test/CodeGenCXX/mangle-ms-cxx11.cpp
329 ↗(On Diff #265046)

I see, this incorporate the field name x here, explaining the FieldDecl check.

340 ↗(On Diff #265046)

I think these should not change, because they are parameters, and in this particular area we are attempting to follow the Itanium mangling strategy. Even if clang is not ABI compatible with MSVC for this feature, I would prefer it if we didn't break ABI compatibility with old versions of clang when possible.

zequanwu updated this revision to Diff 265388.Wed, May 20, 4:58 PM
zequanwu edited the summary of this revision. (Show Details)

Check LambdaContextDecl is not ParamVarDecl.

zequanwu marked an inline comment as done.Wed, May 20, 4:59 PM
zequanwu updated this revision to Diff 267057.Thu, May 28, 3:12 PM

Don't mangle ParmVarDecl.

zequanwu edited reviewers, added: majnemer; removed: rnk.Thu, May 28, 3:13 PM