This is an archive of the discontinued LLVM Phabricator instance.

[mangle] Fix mangling where an extra mangle context is required.
ClosedPublic

Authored by hliao on Oct 9 2019, 10:03 AM.

Details

Summary
  • [Itanium C++ ABI][1], for certain contexts like default parameter and etc., mangling numbering will be local to the particular argument in which it appears.
  • However, for these cases, the mangle numbering context is allocated per expression evaluation stack entry. That causes, for example, two lambdas defined/used understand the same default parameter are numbered as the same value and, in turn, one of them is not generated at all.
  • In this patch, an extra mangle numbering context map is maintained in the AST context to map taht extra declaration context to its numbering context. So that, 2 different lambdas defined/used in the same default parameter are numbered differently.
  • Minor coding change to perfer using tuple for multiple return values.

[1]: https://itanium-cxx-abi.github.io/cxx-abi/abi.html

Event Timeline

hliao created this revision.Oct 9 2019, 10:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 10:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The refactoring of the getCurrentMangleNumberContext API mixed with functional changes makes this much harder to review quickly. Please separate it out.

clang/lib/Sema/SemaLambda.cpp
358

If I'm following correctly, the core of the change is right here? Instead of grabbing a context from the ExpressionEvaluationContextRecord directly, we instead grab the current decl from it, and look up the context from that? That makes sense.

hliao updated this revision to Diff 224115.Oct 9 2019, 11:45 AM

remove refactoring code

hliao marked an inline comment as done.Oct 9 2019, 11:46 AM
hliao added inline comments.
clang/lib/Sema/SemaLambda.cpp
358

yeah, that's the code fix. Lots of changes are due to removing unused code due to this change.

hliao added a comment.Oct 9 2019, 11:49 AM

s/code/core/ in last comment

This revision is now accepted and ready to land.Oct 9 2019, 12:00 PM
hliao added a comment.Oct 9 2019, 12:05 PM

LGTM

Thanks. BTW, shall I commit the refactoring code directly. That change just replaces the code returning multiple values from pointer/reference argument with a tuple.

This revision was automatically updated to reflect the committed changes.