[ARM] Fix several state persistence bugs

[loladiro]

Summary

This fixes three bugs, in all of which state is not or incorrecly reset between
object (i.e. when reusing the same pass manager to create multiple object files)

1. AttributeSection needs to be reset to nullptr, because otherwise the backend will try to emit into the old object file's attribute section causing a segmentation fault.
2. MappingSymbolCounter needs to be reset, otherwise the second object file will start where the first one left off.
3. The MCStreamer base class resets the Streamer's e_flags settings. Since EF_ARM_EABI_VER5 is set on streamer creation, we need to set it again after the MCStreamer was rest.

Also rename Reset (uppser case) to EHReset to avoid confusion with
reset (lower case).

[rengolin]

Please, add comments to the reset methods, since they're all closely named.

Also, write up a better commit message, to explain what kind of bugs were you seeing.

Finally, the test, being a simple pass/fail, could also have some longer explanation of what you're not expecting to see here. This could help people later when they find bugs on that test for unrelated changes.

[loladiro]

Address review comments by expanding some code comments.

[loladiro]

Fix a minor formatting issue

[rengolin]

Apart from the small comment, this looks good to me.

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

1068 ↗

(On Diff #44343)

Can't this be changed by a command line flag? Shouldn't we clear based on that?

[loladiro]

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

1068 ↗

(On Diff #44343)

I was basing this on https://github.com/llvm-mirror/llvm/blob/e5716c4e3ac95bb5f204390720af3a186814f2cb/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp#L1368, which looks unconditional to me. Is there a place where this gets reset later by a flag?

[rengolin]

LGTM. Thanks!

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

1068 ↗

(On Diff #44343)

Hum, it seems not. Whenever someone worries about that, a quick search for EF_ARM_EABI_VER5 will bring them here, and the solution will be whatever it's used there, too.

[psmith]

I think that there is still some state that is not properly reset. If I use a slightly more complicated test case derived from:
int data = 10;

int func(void)
{

return data;

}
clang -c -emit-llvm t.c --target=arm-none-eabi -mcpu=arm7tdmi -o t.bc
llvm-dis -o test.ll t.bc
llc -compile-twice -filetype=obj test.ll -o twice.o
Running the pass manager twice changed the output.
Writing the result of the second run to the specified output
To generate the one-run comparison binary, just run without
the compile-twice option

Looking at the object files it looks like the discrepancies are in the mapping symbols.

I think that this is caused by LastMappingSymbols retaining state across passes. In my local build adding LastMappingSymbols.clear() to void ARMELFStreamer::reset() appears to solve the problem.

I can also reproduce the problem with the AARCH64 backend, also mapping symbol related although I haven't attempted to fix that locally. Would you be able to take another look?

Thanks in advance

Peter

llvm/trunk/include/llvm/MC/MCStreamer.h

public:
   virtual void emitInst(uint32_t Inst, char Suffix = '\0');
 
   virtual void AnnotateTLSDescriptorSequence(const MCSymbolRefExpr *SRE);
 
   virtual void emitThumbSet(MCSymbol *Symbol, const MCExpr *Value);
 
   void finish() override;
 
+  /// Reset any state between object emissions, i.e. the equivalent of
+  /// MCStreamer's reset method.
+  virtual void reset();
+
   /// Callback used to implement the ldr= pseudo.
   /// Add a new entry to the constant pool for the current section and return an
   /// MCExpr that can be used to refer to the constant pool location.
   const MCExpr *addConstantPoolEntry(const MCExpr *, SMLoc Loc);
 
   /// Callback used to implemnt the .ltorg directive.
   /// Emit contents of constant pool for the current section.
   void emitCurrentConstantPool();

llvm/trunk/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

private:
   void finishAttributeSection() override;
   void emitLabel(MCSymbol *Symbol) override;
 
   void AnnotateTLSDescriptorSequence(const MCSymbolRefExpr *SRE) override;
   void emitThumbSet(MCSymbol *Symbol, const MCExpr *Value) override;
 
   size_t calculateContentSize() const;
 
+  // Reset state between object emissions
+  void reset() override;
+
 public:
   ARMTargetELFStreamer(MCStreamer &S)
     : ARMTargetStreamer(S), CurrentVendor("aeabi"), FPU(ARM::FK_INVALID),
       Arch(ARM::AK_INVALID), EmittedArch(ARM::AK_INVALID),
       AttributeSection(nullptr) {}
 };
 
 /// Extend the generic ELFStreamer class so that it can emit mapping symbols at

 class ARMELFStreamer : public MCELFStreamer {
 public:
   friend class ARMTargetELFStreamer;
 
   ARMELFStreamer(MCContext &Context, MCAsmBackend &TAB, raw_pwrite_stream &OS,
                  MCCodeEmitter *Emitter, bool IsThumb)
       : MCELFStreamer(Context, TAB, OS, Emitter), IsThumb(IsThumb),
         MappingSymbolCounter(0), LastEMS(EMS_None) {
-    Reset();
+    EHReset();
   }
 
   ~ARMELFStreamer() {}
 
   void FinishImpl() override;
 
   // ARM exception handling directives
   void emitFnStart();
private:
   }
 
   void EmitThumbFunc(MCSymbol *Func) override {
     getAssembler().setIsThumbFunc(Func);
     EmitSymbolAttribute(Func, MCSA_ELF_TypeFunction);
   }
 
   // Helper functions for ARM exception handling directives
-  void Reset();
+  void EHReset();
+
+  // Reset state between object emissions
+  void reset() override;
 
   void EmitPersonalityFixup(StringRef Name);
   void FlushPendingOffset();
   void FlushUnwindOpcodes(bool NoHandlerData);
 
   void SwitchToEHSection(const char *Prefix, unsigned Type, unsigned Flags,
                          SectionKind Kind, const MCSymbol &Fn);
   void SwitchToExTabSection(const MCSymbol &FnStart);
void ARMTargetELFStreamer::emitThumbSet(MCSymbol *Symbol, const MCExpr *Value) {
   getStreamer().EmitThumbFunc(Symbol);
   getStreamer().EmitAssignment(Symbol, Value);
 }
 
 void ARMTargetELFStreamer::emitInst(uint32_t Inst, char Suffix) {
   getStreamer().emitInst(Inst, Suffix);
 }
 
+void ARMTargetELFStreamer::reset() { AttributeSection = nullptr; }
+
 void ARMELFStreamer::FinishImpl() {
   MCTargetStreamer &TS = *getTargetStreamer();
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
   ATS.finishAttributeSection();
 
   MCELFStreamer::FinishImpl();
 }
 
+void ARMELFStreamer::reset() {
+  MCTargetStreamer &TS = *getTargetStreamer();
+  ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
+  ATS.reset();
+  MappingSymbolCounter = 0;
+  MCELFStreamer::reset();
+  // MCELFStreamer clear's the assembler's e_flags. However, for
+  // arm we manually set the ABI version on streamer creation, so
+  // do the same here
+  getAssembler().setELFHeaderEFlags(ELF::EF_ARM_EABI_VER5);
+}
+
 inline void ARMELFStreamer::SwitchToEHSection(const char *Prefix,
                                               unsigned Type,
                                               unsigned Flags,
                                               SectionKind Kind,
                                               const MCSymbol &Fn) {
   const MCSectionELF &FnSection =
     static_cast<const MCSectionELF &>(Fn.getSection());
 
SwitchToEHSection(".ARM.exidx", ELF::SHT_ARM_EXIDX,
                     SectionKind::getData(), FnStart);
 }
 void ARMELFStreamer::EmitFixup(const MCExpr *Expr, MCFixupKind Kind) {
   MCDataFragment *Frag = getOrCreateDataFragment();
   Frag->getFixups().push_back(MCFixup::create(Frag->getContents().size(), Expr,
                                               Kind));
 }
 
-void ARMELFStreamer::Reset() {
+void ARMELFStreamer::EHReset() {
   ExTab = nullptr;
   FnStart = nullptr;
   Personality = nullptr;
   PersonalityIndex = ARM::EHABI::NUM_PERSONALITY_INDEX;
   FPReg = ARM::SP;
   FPOffset = 0;
   SPOffset = 0;
   PendingOffset = 0;
uint64_t Intval = Opcodes[0] |
                       Opcodes[3] << 24;
     EmitIntValue(Intval, Opcodes.size());
   }
 
   // Switch to the section containing FnStart
   SwitchSection(&FnStart->getSection());
 
   // Clean exception handling frame information
-  Reset();
+  EHReset();
 }
 
 void ARMELFStreamer::emitCantUnwind() { CantUnwind = true; }
 
 // Add the R_ARM_NONE fixup at the same position
 void ARMELFStreamer::EmitPersonalityFixup(StringRef Name) {
   const MCSymbol *PersonalitySym = getContext().getOrCreateSymbol(Name);
 

llvm/trunk/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp

 
 void ARMTargetStreamer::emitCurrentConstantPool() {
   ConstantPools->emitForCurrentSection(Streamer);
 }
 
 // finish() - write out any non-empty assembler constant pools.
 void ARMTargetStreamer::finish() { ConstantPools->emitAll(Streamer); }
 
+// reset() - Reset any state
+void ARMTargetStreamer::reset() {}
+
 // The remaining callbacks should be handled separately by each
 // streamer.
 void ARMTargetStreamer::emitFnStart() {}
 void ARMTargetStreamer::emitFnEnd() {}
 void ARMTargetStreamer::emitCantUnwind() {}
 void ARMTargetStreamer::emitPersonality(const MCSymbol *Personality) {}
 void ARMTargetStreamer::emitPersonalityIndex(unsigned Index) {}
 void ARMTargetStreamer::emitHandlerData() {}

llvm/trunk/test/MC/ARM/twice.ll

+; Check for state persistence bugs in the ARM MC backend
+; This should neither fail (in the comparison that the second object
+; is bit-identical to the first) nor crash. Either failure would most
+; likely indicate some state that is not properly reset in the
+; appropriate ::reset method.
+; RUN: llc -compile-twice -filetype=obj %s -o -
+
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "armv4t-unknown-linux-gnueabi"