Page MenuHomePhabricator

AST: mangle BlockDecls under MS ABI
ClosedPublic

Authored by compnerd on Jun 22 2017, 11:45 AM.

Details

Summary

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 shadowed. Furthermore, using this for the name allows for undname to properly undecorated the name for us. It shows up as the synthetic __block_invocation function that the compiler emitted in the local scope.

Diff Detail

Event Timeline

compnerd created this revision.Jun 22 2017, 11:45 AM
compnerd added a reviewer: eli.friedman.

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.

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.

@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.

@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 with internal linkage, not linkonce. But even if we did use linkonce linkage, the block could get inlined.

compnerd updated this revision to Diff 103768.Jun 23 2017, 1:51 PM

Add additional test cases, improve coverage and mangling.

@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?

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.

@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 for local storage in a block, which is scoped to the block invocation function, which is TU local, and will be referenced by the block_literal (which contains the block invocation function and is dispatched via the BlocksRuntime).

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.github.io/cxx-abi/abi.html#closure-types , specifically the list of cases where lambda types are required to correspond.

compnerd updated this revision to Diff 103794.Jun 23 2017, 4:13 PM

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.

majnemer added inline comments.Jun 23 2017, 4:41 PM
lib/AST/MicrosoftMangle.cpp
974–977

I think you want to use mangleArtificalTagType here.

Ah, thanks for the explanation @efriedma.

lib/AST/MicrosoftMangle.cpp
974–977

I was considering that. The addition of the Discriminator makes that harder. I can create a local buffer and create the name and then mangle that though if you feel strongly about that.

majnemer added inline comments.Jun 23 2017, 4:45 PM
lib/AST/MicrosoftMangle.cpp
974–977

I do, it will ensure it correctly backreferences.

compnerd updated this revision to Diff 103845.Jun 24 2017, 10:43 AM

Address feedback. Also fix the case that was previously not working. This now covers all the various cases that have been discussed.

majnemer added inline comments.Jun 24 2017, 4:39 PM
lib/AST/MicrosoftMangle.cpp
985

Should this be Out << '?' << mangleSourceName(Discriminate("_block_invoke", Discriminator));

990–994

You should probably add comments explaining these bits.

compnerd updated this revision to Diff 103879.Jun 25 2017, 4:14 PM
compnerd marked 2 inline comments as done.

Use mangleSourceName

We need tests that show that it does the right thing in blocks defined in classes in classes and other nested concepts.

lib/AST/MicrosoftMangle.cpp
980–981

Why isn't it local?

991–992

Can blocks not be given a specific calling convention?

993–994

Shouldn't we also mangle an 'E' in here on 64-bit platforms?

999–1001

This logic should be explained.

1003–1005

This logic should be explained.

I can add the nested/nested classes. What were other nested concepts you thinking of?

lib/AST/MicrosoftMangle.cpp
980–981

This code path is global blocks only AFAIK.

991–992

Not AFAIK. The synthetic function here is dispatched through the BlocksRuntime and is assumed to be cdecl.

993–994

I suppose that the default handling in x86_64 would give that. I don't have a strong enough opinion on that. I can add that if you think it makes a difference.

compnerd updated this revision to Diff 103887.Jun 25 2017, 9:20 PM

Some more comments, add test case.

majnemer added inline comments.Jun 25 2017, 9:35 PM
lib/AST/MicrosoftMangle.cpp
993–994

It is consistent with other pointer manglings; I think it'd be best to mangle pointer types as being 64-bit consistently.

compnerd updated this revision to Diff 103981.Jun 26 2017, 10:11 AM

__ptr64 mangling, add tests for 32-bit.

majnemer accepted this revision.Jun 26 2017, 11:42 AM

Looks good to me but I definitely want to hear what @efriedma has to say.

This revision is now accepted and ready to land.Jun 26 2017, 11:42 AM
efriedma added inline comments.Jun 26 2017, 12:35 PM
lib/AST/MicrosoftMangle.cpp
988

The call to mangleName() looks a little weird... I would have expected a call to mangleUnqualifiedName or something like that. (If there's some reason for the asymmetry, a brief comment explaining it would be fine.)

1008

This isn't quite the same thing the Itanium mangling does...

Itanium has special cases for in-class initializers and default arguments. Using the name is fine for in-class initalizers, but it doesn't really work for default arguments: arguments aren't required to have names. Consider, for example:

extern int e(void);
class C {
  void m(int = ^{ static int i = e(); return ++i; }(),
         int = ^{ static int i = e(); return ++i; }());
};
test/CodeGenCXX/msabi-blocks.cpp
91

The Itanium ABI document lists five cases where the mangling is externally visible. I think this is missing a testcase for the "initializers of nonspecialized static members of template classes" case. (Something like "template<typename T> class X { static T foo; }; template<typename T> T X<T>::foo = ^{static int i = e(); return ++i;}();".)

compnerd marked 2 inline comments as done.Jun 26 2017, 2:30 PM
compnerd added inline comments.
lib/AST/MicrosoftMangle.cpp
988

Accounting for the parameter like we do for lambdas makes this unnecessary.

1008

Added a test case and handled it.

test/CodeGenCXX/msabi-blocks.cpp
91

This is currently broken even under the itanium scheme. I think that doing that in a follow up is reasonable.

template <typename T>
struct s {
  static T i;
};

template <typename T>
T s<T>::i = ^{ static T i = T(); return i; }();

template class s<int>;
template class s<short>;
efriedma added inline comments.Jun 26 2017, 2:53 PM
test/CodeGenCXX/msabi-blocks.cpp
91

Fine; please file a bug.

compnerd marked 2 inline comments as done.Jun 26 2017, 3:02 PM
compnerd added inline comments.
lib/AST/MicrosoftMangle.cpp
988

Err, still need it for the fields; added a comment.

compnerd updated this revision to Diff 104028.Jun 26 2017, 3:08 PM

Handle unnamed parameters, improve mangling for NSDMis. The one case that we dont handle currently causes an assertion even in itanium mode.

LGTM (with the caveat that I don't know anything about Microsoft mangling).

compnerd closed this revision.Jun 26 2017, 4:45 PM

SVN r306347

Thanks @efriedma and @majnemer!