Page MenuHomePhabricator

WinEH for unnamed functions
Needs RevisionPublic

Authored by pbrunet on Sep 3 2017, 11:57 PM.

Details

Reviewers
rnk
majnemer
Summary

dtor and catch symbol are required for windows exception handling. These symboles were defined based on function name which is incorrect in the case of unnamed functions as we may have multiple functions with the same empty name.
It leads to assert if they are available or crash on the exception path.

Diff Detail

Event Timeline

pbrunet created this revision.Sep 3 2017, 11:57 PM

I feel like the test here could use some simplification to make it clearer what you're testing. If there's any IR in there that you don't *absolutely* need, it'd be good to remove it. This makes it easier for others to debug failures in the future.

test/CodeGen/WinEH/wineh-unnamed.ll
2

Spelling and grammar could use some fixing here.

103

You can remove the ; Function Attrs... comments. Basically any comment that you didn't write for your test can go.

117

You don't need comments like the ; preds... lines in tests

153

Do you need all of these attributes for your test? If you don't, it would simplify it a lot to remove some of them.

I also suggest adding some reviewers to your patch! If you look at the blamelist of WinException.cpp you'll probably find some people that know the code well that can look at your stuff. I'm not a Windows expert, but if I was I'd give you a proper review. :)

pbrunet added reviewers: rnk, majnemer.

Thanks for the review and sorry for the delay.

I Updated this patch with :

  • handle new API : replace getRealLinkageName by dropLLVMManglingEscape
  • Reduce test case.
rnk requested changes to this revision.Jan 17 2018, 10:35 AM

The same logic would need to be applied to the X86ISelLowering call site of MCContext::getOrCreateParentFrameOffsetSymbol. I think it would be preferable to manually number and provide names for nameless functions that use funclet personalities in WinEHPrepare.

This revision now requires changes to proceed.Jan 17 2018, 10:35 AM
In D37434#978881, @rnk wrote:

The same logic would need to be applied to the X86ISelLowering call site of MCContext::getOrCreateParentFrameOffsetSymbol. I think it would be preferable to manually number and provide names for nameless functions that use funclet personalities in WinEHPrepare.

Heh, I was just about to recommend using something like Mangler::getNameWithPrefix (with CannotUsePrivateLabel set to true) into a raw_svector_ostream. This way we get a name which will correspond to what we get in the object file. We would only use Mangler::getNameWithPrefix if the GV had no name.

rnk added a comment.Jan 17 2018, 11:19 AM
In D37434#978881, @rnk wrote:

The same logic would need to be applied to the X86ISelLowering call site of MCContext::getOrCreateParentFrameOffsetSymbol. I think it would be preferable to manually number and provide names for nameless functions that use funclet personalities in WinEHPrepare.

Heh, I was just about to recommend using something like Mangler::getNameWithPrefix (with CannotUsePrivateLabel set to true) into a raw_svector_ostream. This way we get a name which will correspond to what we get in the object file. We would only use Mangler::getNameWithPrefix if the GV had no name.

I think part of the trouble here is that I put the logic for computing the label name in MCContext, which cannot use IR constructs. Having this in the Mangler would work better.