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

[Jason Kim]

On Tue, Oct 19, 2010 at 8:01 AM, Rafael Espindola <espindola at google.com> wrote:
>> Okay, I just committed the convert (all numbers from elf-dump are now
>> hex) as r116753.
>
> I noticed that was reverted. The first thing is to fix it and commit
> again. Once it is in:
>
> +
> +    case ELF::SHT_ARM_EXIDX:
> +    case ELF::SHT_ARM_PREEMPTMAP:
> +      assert(0 && "FIXME: sh_type value not supported!");
> +      break;
> +
> +    case ELF::SHT_ARM_ATTRIBUTES:
> +      break;
> +
> +    case ELF::SHT_ARM_DEBUGOVERLAY:
> +    case ELF::SHT_ARM_OVERLAYSECTION:
>     default:
>       assert(0 && "FIXME: sh_type value not supported!");
>       break;
>
> The two new cases are exactly like the default. Same for the last two
> new one. Can you just use the default? Another option is to list every
> enum value and remove the default so that we get a warning if one is
> missing. Not sure if there are two many for this to be practical.


It is new code, and the 5 cases I added (in order of their numeric
values) are the complete set of ARM specific sections.
I guess having only active ones be 'cased' makes sense too. I'll do
your first suggestion.
I removed the 3 dangling explicitly unsupported cases, and have them
fall through to default.
Now only explicitly supported cases are listed in the switch.

Hope this is OK.

>
> It still looks like you are creating more class members than you need
> to. Take headFragment for example. It is only assigned in one place in
> the code, emitARMAttributeSection. emitARMAttributeSection is only
> called once in emitAttributes. emitARMAttributeSection can "return"
> the headFragment and that can then be passed to fixupAttrSizes.

I humbly beg to differ on this one :-)

Since emitARMAttributeSection() gets called both for .s and .o, I
don't think it makes sense for it to a MCDataFragment when it is
emitting a .s

It seems cleaner for it to just return nothing and save some state (in
case of .o emission only) instead of passing a bunch of pointer values
trampoline fashion in case of .o emission to save some slots.

Even if it were to return the headFragment, what about the
currFileFragment? (and possibly the currSectionFragment and
currSymbolFragment in the future?)
The currFileFragment is needed elsewhere, so it makes sense to save it
somewhere.
If it makes sense to store one blob, why not its container?

Does this make sense? Am I missing something?

> Cheers,
> --
> Rafael Ávila de Espíndola
>

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-mc-elf-s06.patch6
Type: application/octet-stream
Size: 12331 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101019/d21d5ca4/attachment.obj>