Fix `FaultMaps` crash when the out streamer is reused

[yuyichao]

Summary

Make sure the map is cleared before processing a new module. Similar to what is done on StackMaps.

This issue is similar to D38588, though this time for FaultMaps (on x86) rather than ARM/AArch64. Other than possible mixing of information between modules, the crash is caused by the pointers values in the map that was allocated by the bump pointer allocator that is unwinded when emitting the next file. This issue has been around since 3.8 though only the recent experience with D38588 helped me finding the fix....

This issue is likely much harder to write a test for since AFAICT it requires emitting something much more compilcated (and possibly real code) instead of just some random bytes....

[sanjoy]

lib/Target/X86/X86AsmPrinter.h

137 ↗

(On Diff #119043)

Does it make sense to do these things in doFinalization?

[yuyichao]

lib/Target/X86/X86AsmPrinter.h

137 ↗

(On Diff #119043)

As I said, I'm mainly following SM here.

That said, I do think something needs to be done here. IIUC, these maps holds information about the module so it makes sense to initialize them when the pass is going to handle a new module. It might also be good to also reset them in reset so that (assuming reset is called at the same time the MCContext is reset) the map won't hold any invalid pointers. I didn't also do that because SM didn't and the invalid pointers themselves won't cause a crash as long as they are cleared before the run starts.

[sanjoy]

lib/Target/X86/X86AsmPrinter.h

137 ↗

(On Diff #119043)

Ok, that sgtm; but I want @skatkov to take a look.

[yuyichao]

Thanks for the review.

[skatkov]

lib/Target/X86/X86AsmPrinter.h

137 ↗

(On Diff #119043)

I guess for clean solution it should really good to update reset and doFinalization to avoid seeing the invalid pointers while they still do not cause a crash themselves. But the point that SM does not do that makes me thinking that it might be a separate patch.

[yuyichao]

But the point that SM does not do that makes me thinking that it might be a separate patch.

Exactly what I was thinking so I'd like to leave that to someone more certain than me about when reset is called/how it is used.

Thanks for reviewing. Will push in a day or so.

llvm/trunk/include/llvm/CodeGen/FaultMaps.h

public:
   };
 
   explicit FaultMaps(AsmPrinter &AP);
 
   static const char *faultTypeToString(FaultKind);
 
   void recordFaultingOp(FaultKind FaultTy, const MCSymbol *HandlerLabel);
   void serializeToFaultMapSection();
+  void reset() {
+    FunctionInfos.clear();
+  }
 
 private:
   static const char *WFMP;
 
   struct FaultInfo {
     FaultKind Kind = FaultKindMax;
     const MCExpr *FaultingOffsetExpr = nullptr;
     const MCExpr *HandlerOffsetExpr = nullptr;

llvm/trunk/lib/Target/X86/X86AsmPrinter.h

bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              raw_ostream &OS) override;
 
   /// \brief Return the symbol for the specified constant pool entry.
   MCSymbol *GetCPISymbol(unsigned CPID) const override;
 
   bool doInitialization(Module &M) override {
     SMShadowTracker.reset(0);
     SM.reset();
+    FM.reset();
     return AsmPrinter::doInitialization(M);
   }
 
   bool runOnMachineFunction(MachineFunction &F) override;
   void EmitFunctionBodyStart() override;
   void EmitFunctionBodyEnd() override;
 };
 
 } // end namespace llvm
 
 #endif