[llvm-commits] ARM ELF disassembly with integrated-as

[Greg Fitzgerald]

Thanks Tim, some great additions here.


> I've also gone back to the original patch's style of making
> MCELFStreamer a publicly visible rather than tacking ARMELFStreamer
> onto the end of MCELFStreamer.cpp

I like that style better too.  I felt tacking ARMELFStreamer onto
MCELFStreamer was better for communicating what functionality changed, and
planned to move the files in a second patch.  Now that were all on the same
page, it makes no difference.  +1


> The fragility is my main worry. The patch rather relies on the fact
> that EmitBytes and EmitValueImpl are the only functions which will
> actually emit $d data into a section. Unfortunately, there's no
> obvious way to assert on that so tests are the only option.

True, but I prefer your solution as well.  Using the DataRegion methods was
less fragile coming from the IR side, but your solution handles both IR and
assembly with the same mechanism.  +1


+  virtual void ChangeSection(const MCSection *Section) {
+    // We have to keep track of the mapping symbol state of any sections we
+    // use. Each one should start off as EMS_None, which is provided as the
+    // default constructor by DenseMap::lookup.
+    LastMappingSymbols[getPreviousSection()] = LastEMS;
+    LastEMS = LastMappingSymbols.lookup(Section);
+
+    MCELFStreamer::ChangeSection(Section);
+  }

I like it, thank you.  I see that you keep the LastEMS state variable
around and therefore only need to do the lookup during ChangeSection() and
not in every call to EmitDataMappingSymbol().

LGTM!

-Greg




On Mon, Dec 3, 2012 at 8:20 AM, Tim Northover <t.p.northover at gmail.com>wrote:

> Hi,
>
> I've done some work on this mapping symbols patch. I think it works
> properly for both CodeGen and assembly now, with all the substantial
> changes isolated to an ARMELFStreamer.
>
> I've also gone back to the original patch's style of making
> MCELFStreamer a publicly visible rather than tacking ARMELFStreamer
> onto the end of MCELFStreamer.cpp. I think this is a neater solution
> even in this case, and will get even more so when AArch64 lands and
> needs to do something similar.
>
> > FWIW. It'll still be a whole lots of things to change and a fair bit of
> fragility
>
> The fragility is my main worry. The patch rather relies on the fact
> that EmitBytes and EmitValueImpl are the only functions which will
> actually emit $d data into a section. Unfortunately, there's no
> obvious way to assert on that so tests are the only option.
>
> Ok to commit?
>
> Tim.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121203/465300bd/attachment.html>