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

[Jason Kim]

On Tue, Oct 12, 2010 at 7:53 AM, Rafael Espindola <espindola at google.com> wrote:
>> arm-mc-elf-s06-elfdump.patch modifies elf-dump script.
>> It adds a --hex mode for printing all numbers to hex format.
>> AFAIK, elf-dump output without --hex has remained the same.
>
> Not sure if I like the idea of having the option. I do like the idea
> of printing in hex. If you have a good argument for why we should have

I hope you are kidding about that! Do you like converting between hex
and decimal just for testing?
I added it just for sanity checking the magic constants from the arch.
It sucks to have to convert 0x70000003 to decimal and back again, just
for testing purposes (and then forgetting two weeks later what the
magic number was supposed to be!
0x700000003 is recognizable. The decimal form isn't (unless you are strange! :-)

> the option, I would suggest just having a global variable with 'int'
> or 'hex' and FormatOutput can then just be a plain function.

Hmm, yeah, might as well as do it that way - For some reason, the %
operator seems to interfere with "%s %s" % Foo(a, b) versus "%s"
%Foo(a), "%s"%Foo(b),
so having a class to do mass convert of all args didn't work.
I'll fix it.

>
>> The second patch arm-mc-elf-s06.patch changes finally adds enough code to
>> directly test ARM/MC/ELF/.o emission.
>> The newly added test passes but the resulting ELF is by no means correct.
>> It is a simple step in the right direction.
>
> What is not correct about it? I hope that the items you have CHECK for
> are correct :-)

Yes, except for the subsection offsets. There's also .cpu related
cruft that is missing. As they say, one step at a time.... :-)
I guess I should have described it as "Substantially correct, with
additional details remaining to be worked out"

> Is enough of the asm parser working for
> 2010-10-08-mc-asm-header-obj-test.ll to be written in assembly and use
> llvm-mc?

I haven't tested it that way yet. I suppose I'll give it a shot.
Regardless, I'd argue that it's beyond the scope of these two patches.

> That is a long test filename btw.

Only 3 chars longer than 2010-09-29-mc-asm-header-test.ll :-)

> +  // Fixme: All this is duplicated in MCSectionELF. Why??
> +  // Exception Index table
>
> Is it? I cannot find any references to EXIDX for example.

Much of the ELF specific flags are repeated on both files. I don't
know why. Fixing that is beyond scope of these two patches.

> How much work is it to complete WriteNopData? If you write the test in
> assembly, can the assert stay?

I think I'll separate that out into 1+ new patches - My next patch in
sequence  was to fill out enough of the ARMMCEncode to handle mov r0,
#0 and bx lr
So might as well as add in Nop generation there as well

> +    MCObjectStreamer *ELFStreamer;
> +    MCAssembler *Asm;
> +    MCSectionData *AttrSecData;
> +    MCDataFragment *headFragment
>
> Do you really need all this in here? I don't see something similar in
> X86 for example...

The are not there in X86, because there are no X86 specific sections
in the X86 ELF ABI! (to be more precise, ELF existed first for X86, so
by definition, it need not any specific sections just for it.)
ARM requires it. In order to add a ELF section outside of the
MCAssembler/MCELFStreamer, I needed to have those intermediates there
anyway. Either regenerate them every time I need it, or save it. I
chose the latter.
MC seem to do a good job of unifying between ELF/COFF/MachO, but not
everything can be neatly lifted that way. An ARCH specific section
seems to be one of these.

>
>> As always, feedback/corrections are very welcome.
>>
>> -jason
>
> Thanks,
> --
> Rafael Ávila de Espíndola
>


So I guess I'll do the following.
1. Fix up elf-dump
2. Fix up Nop generation
3. Reland this patch in sequence.

Does this sound like a sane plan?


Thanks for feedback!
-Jason