Page MenuHomePhabricator

[PowerPC] Clear the set of symbols that need to be updated in MCTargetStreamer
ClosedPublic

Authored by nemanjai on Apr 15 2020, 5:09 AM.

Details

Summary

We have added code to correct the .localentry values on assignments. However, we never clear the set so presumably it will still contain the (now dangling) MCSymbol pointers across a call to finish() and reset() in the streamer.

This is based on my speculation that it is the reason we are getting segmentation faults mentioned in https://bugs.llvm.org/show_bug.cgi?id=45366

I will work on providing a test case, but I am posting here so the OP of the mentioned PR can try this out and perhaps generate a reproducer.

Diff Detail

Event Timeline

nemanjai created this revision.Apr 15 2020, 5:09 AM
luporl accepted this revision.Apr 15 2020, 6:47 AM

Clearing UpdateOther in finish() seems correct.
I've not considered the reuse of PPCTargetELFStreamer objects in the .localentry fix.

This revision is now accepted and ready to land.Apr 15 2020, 6:47 AM
vchuravy accepted this revision.Apr 15 2020, 6:49 AM
vchuravy added a subscriber: vchuravy.

I am unable to reproduce the issue with this patch applied. 👍

MaskRay requested changes to this revision.Apr 15 2020, 8:33 AM

Isn't it a Julia usage error? After MCStreamer::Finish, MCStreamer isn't supposed to be reused. In LLVM code, it is definitely not...

This revision now requires changes to proceed.Apr 15 2020, 8:33 AM

We do reuse the constructed pass pipeline more than once, but that is a supported configuration see -compile-twice in LLC (as an example https://reviews.llvm.org/D17712)

Isn't it a Julia usage error? After MCStreamer::Finish, MCStreamer isn't supposed to be reused. In LLVM code, it is definitely not...

Are you sure about this? Why does MCStreamer::reset() exist?

MaskRay added a comment.EditedApr 15 2020, 11:34 AM

Isn't it a Julia usage error? After MCStreamer::Finish, MCStreamer isn't supposed to be reused. In LLVM code, it is definitely not...

Are you sure about this? Why does MCStreamer::reset() exist?

Then it seems the cleanup should be placed in a derived function of MCStreamer::reset()?

Then it seems the cleanup should be placed in a derived function of MCStreamer::reset()?

MCTargetStreamer is a member of MCStreamer not a class derived from it. Of course, we could do something similar to what ARMTargetStreamer does and provide virtual void reset() that the derived classes can then implement and override and that MCStreamer::reset() would call. Since this does not appear to have been needed elsewhere before, this seems unnecessary, but I am certainly not against extending MCTargetStreamer this way if that is preferred.
Would you prefer that I do that?

MaskRay accepted this revision.Apr 15 2020, 1:01 PM

LGTM.

This revision is now accepted and ready to land.Apr 15 2020, 1:01 PM
This revision was automatically updated to reflect the committed changes.