[llvm-commits] Proof of concept patch for unifying the .s/ELF emission of .ARM.attributes

Also what is the preferred method for MC way of setting out subsection
sizes after the fact? I am guessing I need to use an MCFixup?
How do I get an MCExpr to evaluate a method for the subsection size?
Is there an equivalent use in the places using MCFixup?
Do I need to add a new subclass to MCExpr for doing this?

JimG, can you please comment on the MachO specific parts in the
ARMAsmPrinter.cpp? Is there an example of a "subsection size" that is
"Fixed up" after all the blobs that go into that subsection are
emitted? The closest examples I could find weren't that close :frowning:
(Matter of fact, for ELF, it looks like the section sizes are actually
calculated after everything has been processed by MCAssembler, and the
headers sizes are not "fixed up" but completely rewritten from the
beginning)

I really don't follow. Please just convert the current patch to use
the existing APIs. If in the next one you really need a missing
feature it will be a lot easier to explain what it is.

Hi Rafael

Included are two patches. The first one s06-mod, is the original one.
The s06-brk uses the high level Streamer interface, but currently does
not pass the test I added.

Please see the comments in the second patch. Hopefully it clarifies
the issues in using the high level MCStreamer interface in emitting
the .ARM.attributes ELF section directly.
It does not seem worthwhile to use the high level Streamer interface
for this kind of work, so I'd very much like some feedback on what
the preferred solution is.

Sigh. I sent an earlier patch by mistake for s06-brk. It does not
compile, but it should still suffice for demonstration purposes.
Sorry for the mess.

OK, after reading the docs I have some extra comments (and an updated patch).

*) To support per section or per symbol attributes we would have to
move this to the processing done in the end of the file. Lets not do
this right now.
*) We don't currently use any string attributes, so I did not implement them.
*) Having an attribute emitter class is a nice way to separate the job
of creating the attributes and the decisions of what attributes to
use.
*) The best way I could find of avoiding code duplication was to make
getCurrentFragment and getOrCreateDataFragment public. If they must
really be protected we can then create a special ARM streamer that has
the same functionality as the attribute emitter in this patch. Yet
another option is adding a method for writing over old data in the
streamer API.
*) I added support for multiple vendors, but that is not used. I can
remove if you want.
*) Using virtual methods might be too much. Let me know if you like
OutStreamer.hasRawTextSupport better. Using the class has the
advantage of having only one cast to MCObjectStreamer.

Cheers,
Rafael

attrs.patch (13.1 KB)

I also noticed that we were trying to optimize the output of 41 bytes
of data :slight_smile:

The attached patch is similar to the previous one but drops the API
changes by just accumulating the attributes locally before outputting
them.

Cheers,
Rafael

attrs.patch (12 KB)

All well and good, Rafael, 3 new classes and 4 new slots.
In any case, I am done arguing this - outside of any additional input
on this might as well as commit your rework. Any objections?

I also noticed that we were trying to optimize the output of 41 bytes
of data :slight_smile:

Err, Yes. 2+ weeks on this. I am without words at the moment.

The attached patch is similar to the previous one but drops the API
changes by just accumulating the attributes locally before outputting
them.

-jason