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

[Jason Kim]

On Mon, Oct 11, 2010 at 9:20 AM, Jim Grosbach <grosbach at apple.com> wrote:
> Hi Jason,
>
> Glad to see this making progress. Here's a bit of general feedback. I'm not an expert on Linux ELF, so I can't really comment on that aspect of things, though.
>
>>  namespace ARMBuildAttrs {
>>    enum AttrType {
>> +    // For the .cpu asm construct
>> +    ARM_CPU,
>> +    // Rest correspond to ELF/.ARM.attributes
>>      File                      = 1,
>>      Section                   = 2,
>>      Symbol                    = 3,
>
>
> Is this new attr distinct from the ones already there for the CPU? CPU_name, in particular, seems a likely candidate. It's hard to tell since the uses of the others don't appear to be implemented yet. In any case, if a new attr does need to be added, it should be consistent with the rest of them. No prefix, and just add the value onto the end (i.e., "Cpu = 71") of the list.

All of the other enums in the ARMBuildAttrs are specific part of (i.e.
can appear as markers in) the .ARM.attributes section.
The new one I added correspond to the .cpu asm attr, which translates
into one or more sequence of CPU_NAME, CPU_ARCH ... in the actual ELF
section, so I needed a way to get that done.
There is no neat correspondence between the asm attrs and what goes
into the .ARM.attributes. Certainly not for the .cpu attribute (its
one to many)

>
>> +void ARMAsmPrinter::emitTextAttribute(ARMBuildAttrs::AttrType attr,
>> +                                      StringRef val) {
>> +  if (attr != ARMBuildAttrs::ARM_CPU) {
>> +    return;
>> +  }
>> +  if (OutStreamer.hasRawTextSupport()) {
>> +    if (val != "generic") {
>> +      OutStreamer.EmitRawText("\t.cpu " + val);
>> +    }
>> +  } else {
>> +    // FIXME: ELF
>> +  }
>> +}
>
>
> Since this looks like it'll be fleshed out to handle the other attributes as well, just make it a switch statement. That's more consistent with the rest of the LLVM codebase. Something like:

>
> switch (attr) {
> default:
>  assert(0 && "Unimplemented build attribute!");
>  break;
> case ARMBuildAttrs::ARM_CPU: {
>  ...
> }
> }

Hokay, at least this part I can do for the others (but prolly not
worth while for the new ARM_CPU tag.)
I'll make the special case transition clear and resend patch.
Thanks!

p.s. I have another patch waiting for this to land - I will be adding
just enough to get a basic two instruction function (thanks for new
scaffolding on EncodeInstuction()  Jim!)  output as ELF .o, and be
somewhat sane -
i.e. ARM/MC/ELF hello world ("misspelled, and broken" but at least it
will be a start. :-)


>
> Regards,
>
>  Jim
>
>
>
> On Oct 7, 2010, at 8:54 PM, Jason Kim wrote:
>
>> Second set of ARM/MC/ELF changes.
>>
>> Added ARM specific ELF section types.
>> Added AttributesSection to ARMElfTargetObject
>> First step in unifying .cpu assembly tag with ELF/.o
>> llc now asserts on actual ELF emission on -filetype=obj :-)
>>
>> Feedback, please!
>>
>> Thanks for reading!
>>
>> -Jason
>> <arm-mc-elf-s05.patch>
>
>