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

[Jim Grosbach]

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.

> +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: {
  ...
}
}


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>