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

[nemanjai]

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.

[luporl]

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

[vchuravy]

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

[MaskRay]

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

[vchuravy]

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)

[nemanjai]

In D78196#1983864, @MaskRay wrote:

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]

In D78196#1984264, @nemanjai wrote:

In D78196#1983864, @MaskRay wrote:

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()?

[nemanjai]

In D78196#1984391, @MaskRay wrote:

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]

LGTM.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp

void emitAssignment(MCSymbol *S, const MCExpr *Value) override {
     else
       UpdateOther.erase(Symbol);
   }
 
   void finish() override {
     for (auto *Sym : UpdateOther)
       if (Sym->isVariable())
         copyLocalEntry(Sym, Sym->getVariableValue());
+
+    // Clear the set of symbols that needs to be updated so the streamer can
+    // be reused without issues.
+    UpdateOther.clear();
   }
 
 private:
   SmallPtrSet<MCSymbolELF *, 32> UpdateOther;
 
   bool copyLocalEntry(MCSymbolELF *D, const MCExpr *S) {
     auto *Ref = dyn_cast<const MCSymbolRefExpr>(S);
     if (!Ref)