compnerd closed this revision.
compnerd added a comment.
SVN r306347
Thanks @efriedma and @majnemer!
https://reviews.llvm.org/D34523
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
efriedma added a comment.
LGTM (with the caveat that I don't know anything about Microsoft mangling).
https://reviews.llvm.org/D34523
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
compnerd updated this revision to Diff 104028.
compnerd added a comment.
Handle unnamed parameters, improve mangling for NSDMis. The one case that we
dont handle currently causes an assertion even in itanium mode.
https://reviews.llvm.org/D34523
Files:
lib/AST/MicrosoftMangle.cpp
test/Cod
compnerd marked 2 inline comments as done.
compnerd added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:988
+ if (const auto *RD = dyn_cast(DC))
+mangleName(RD);
+ else
compnerd wrote:
> efriedma wrote:
> > The call to mangleName() lo
efriedma added inline comments.
Comment at: test/CodeGenCXX/msabi-blocks.cpp:90
+}
+
compnerd wrote:
> efriedma wrote:
> > The Itanium ABI document lists five cases where the mangling is externally
> > visible. I think this is missing a testcase for the "initia
compnerd marked 2 inline comments as done.
compnerd added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:988
+ if (const auto *RD = dyn_cast(DC))
+mangleName(RD);
+ else
efriedma wrote:
> The call to mangleName() looks a little weird..
efriedma added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:988
+ if (const auto *RD = dyn_cast(DC))
+mangleName(RD);
+ else
The call to mangleName() looks a little weird... I would have expected a call
to mangleUnqualifiedName or s
majnemer accepted this revision.
majnemer added a comment.
This revision is now accepted and ready to land.
Looks good to me but I definitely want to hear what @efriedma has to say.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
___
cfe-comm
compnerd updated this revision to Diff 103981.
compnerd added a comment.
__ptr64 mangling, add tests for 32-bit.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
Files:
lib/AST/MicrosoftMangle.cpp
test/CodeGenCXX/msabi-blocks.cpp
Index: test/CodeGenCXX/msabi-blocks.cpp
==
majnemer added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:993-994
+ Out << "YAX";
+ // struct __block_literal *
+ Out << "PA";
+ mangleArtificalTagType(TTK_Struct,
compnerd wrote:
> majnemer wrote:
> > Shouldn't we also mangle an
compnerd updated this revision to Diff 103887.
compnerd added a comment.
Some more comments, add test case.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
Files:
lib/AST/MicrosoftMangle.cpp
test/CodeGenCXX/msabi-blocks.cpp
Index: test/CodeGenCXX/msabi-blocks.cpp
===
compnerd added a comment.
I can add the nested/nested classes. What were other nested concepts you
thinking of?
Comment at: lib/AST/MicrosoftMangle.cpp:980-981
+ unsigned Discriminator = BD->getBlockManglingNumber();
+ if (!Discriminator)
+Discriminator = Co
majnemer added a comment.
We need tests that show that it does the right thing in blocks defined in
classes in classes and other nested concepts.
Comment at: lib/AST/MicrosoftMangle.cpp:980-981
+ unsigned Discriminator = BD->getBlockManglingNumber();
+ if (!Discrimin
compnerd updated this revision to Diff 103879.
compnerd marked 2 inline comments as done.
compnerd added a comment.
Use `mangleSourceName`
Repository:
rL LLVM
https://reviews.llvm.org/D34523
Files:
lib/AST/MicrosoftMangle.cpp
test/CodeGenCXX/msabi-blocks.cpp
Index: test/CodeGenCXX/msabi
majnemer added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:985
+
+ Out << '?' << Discriminate("_block_invoke", Discriminator) << '@';
+ if (const auto *RD = dyn_cast(DC))
Should this be `Out << '?' << mangleSourceName(Discriminate("_block_i
compnerd updated this revision to Diff 103845.
compnerd added a comment.
Address feedback. Also fix the case that was previously not working. This now
covers all the various cases that have been discussed.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
Files:
lib/AST/MicrosoftMangl
majnemer added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:981-984
+ Out << "YAXPAU__block_literal";
+ if (Discriminator)
+Out<< '_' << Discriminator;
+ Out << "@@@Z";
compnerd wrote:
> majnemer wrote:
> > I think you want to u
compnerd added a comment.
Ah, thanks for the explanation @efriedma.
Comment at: lib/AST/MicrosoftMangle.cpp:981-984
+ Out << "YAXPAU__block_literal";
+ if (Discriminator)
+Out<< '_' << Discriminator;
+ Out << "@@@Z";
majnemer wrote:
> I t
majnemer added inline comments.
Comment at: lib/AST/MicrosoftMangle.cpp:981-984
+ Out << "YAXPAU__block_literal";
+ if (Discriminator)
+Out<< '_' << Discriminator;
+ Out << "@@@Z";
I think you want to use mangleArtificalTagType here.
Repo
compnerd updated this revision to Diff 103794.
compnerd added a comment.
This is a step in the right direction. Although the NSDMI cases and default
parameter value cases are not yet handled, they break due to tracking of the
global mangling number tracking, not due to the scheme.
Repository:
efriedma added a comment.
> How do we end up in a state where the block invocation function is
> referenced external to the TU?
ODR allows certain definitions, like class definitions and inline function
definitions, to be written in multiple translation units. See
http://itanium-cxx-abi.git
compnerd added a comment.
@efriedma I think that Im still not understanding the case that you are trying
to point out. How do we end up in a state where the block invocation function
is referenced external to the TU? The block would be referenced to by name of
the block, no? AFAICT, this is
efriedma added a comment.
I think we just use "0" as a sentinel value to indicate the block doesn't need
a mangling number. getLambdaManglingNumber works the same way? See
CXXNameMangler::mangleUnqualifiedBlock in the Itanium mangler.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
compnerd added a comment.
@efriedma which bit of the Itanium mangling should I be looking at? A
BlockDecl does not have visibility associated with them, so Im not sure what I
should be checking to see if the block is visible or not. What is the best way
forward for finishing this up?
Reposi
compnerd updated this revision to Diff 103768.
compnerd added a comment.
Add additional test cases, improve coverage and mangling.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
Files:
lib/AST/MicrosoftMangle.cpp
test/CodeGenCXX/msabi-blocks.cpp
Index: test/CodeGenCXX/msabi-blocks.
efriedma added a comment.
> @efriedma hmm...using getBlockManglingNumber causes the name to be
> duplicated. Ill look into that.
Have you looked at the Itanium mangling implementation?
> However, wouldn't all the block invocation functions be defined and COMDAT'ed?
IIRC, we always emit blocks
compnerd added a comment.
@efriedma hmm...using `getBlockManglingNumber` causes the name to be
duplicated. Ill look into that. However, wouldn't all the block invocation
functions be defined and COMDAT'ed?
@majnemer Sure, will add more tests.
Repository:
rL LLVM
https://reviews.llvm.org/
efriedma added a comment.
Patch is missing context.
You have to use getBlockManglingNumber() for blocks which are externally
visible; otherwise, the numbers won't be consistent in other translation units.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
___
majnemer added a comment.
Can you please have a test where you define blocks w/ static variables in
several default arguments of the same function? Also would be good to have this
in NSDMIs in class definitions.
Repository:
rL LLVM
https://reviews.llvm.org/D34523
compnerd created this revision.
compnerd added a project: clang.
When generating the decorated name for a static variable inside a BlockDecl,
construct a scope for the block invocation function that homes the parameter.
This allows for arbitrary nesting of the blocks even if the variables are
30 matches
Mail list logo
