[PATCH] D49936: [ARM] Support the .isnt directive for MachO and COFF targets

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp:52
 
+void ARMTargetStreamer::emitInst(uint32_t Inst, char Suffix) {
+  unsigned Size;
----------------
mstorsjo wrote:
> peter.smith wrote:
> > This looks almost identical to ARMELFStreamer::emitInst() with just the EmitARMMappingSymbol() and EmitThumbMappingSymbol().
> > 
> > Would it be possible to add a hook for adding the mapping symbol that the ELF backend can override and then make it use the implementation in ARMTargetStreamer?
> Yes, it's identical except for that, and minus some asserts about IsThumb which isn't available here.
> 
> Sharing in one way or another is doable, but it requires a little more than that. The ARMELFTargetStreamer::emitInst() method calls ARMELFStreamer::emitInst(), which contains the body of the method implementation, that we'd like to share. But ARMELFStreamer::EmitBytes() also overrides MCELFStreamer::EmitBytes to force adding a data symbol in front of it. So we can't have ARMTargetStreamer just call getStreamer().EmitBytes(StringRef(Buffer, Size)); since that will get the extra data prefix prepended.
> 
> So we can:
> - Add an emitBytesCode to ARMTargetStreamer which ARMELFTargetStreamer can override, which takes care of adding the arm/thumb mapping symbol and takes care of not adding a data symbol
> - Add getInstBytes to ARMTargetStreamer which both ARMTargetStreamer and ARMELFTargetStreamer can use to get the formatted bytes to print
> - Something else?
Thanks for pointing that out; it is more complicated than I first thought. One possible alternative could be to add an overaload of emitInst with a MCObjectStreamer parameter to ARMTargetStreamer which we could call emitBytes() and potentially add the mapping symbol hooks to. The backend overrides of emitInst could add the appropriate streamer. 

This is getting to the point where the extra code to factor out the duplication may not be worth it. I suggest seeing if anyone else has any preferences.


Repository:
  rL LLVM

https://reviews.llvm.org/D49936