[PATCH] D94072: [MC][ELF] Fix accepting abbreviated form with sh_flags and sh_entsize

[Tobias Burnus via Phabricator via llvm-commits]

burnus added a comment.

In D94072#2480225 <https://reviews.llvm.org/D94072#2480225>, @MaskRay wrote:

> This now becomes hazy. While `.rodata.cst8` is a built-in section name in MC, it is not in GNU as. i.e. If you specify `.section .rdata.cst8`, GNU as does not make it SHF_MERGE or set sh_entsize to 8.

While the assemblers' behavior differ (and, hence, the assemblies differ), I fail to see how this affects the `leave out in subsequent uses of the same sections` feature.

Namely:

- Having twice `.section .rodata.cst8` (without user-specified flags + entsize) is consistent, accepted by all assemblers and won't trigger an/the error. It may produce a different output (SHF_MERGE vs. no) for different assemblers, but each assembler is consistent. (GCC doesn't produce ".rodata" without writing flags for the first .section)
- Having

  .section .rodata.cst8,"aM", at progbits,8
  .section .rodata.cst8

Is also consistent (if no error is triggered, i.e. with this patch or with LLVM 10): entsize = 8 and ELF::SHF_ALLOC | ELF::SHF_MERGE.

That's also consistent between GNU assembler and MC, except that MC sets ELF::SHF_ALLOC twice, once via the ".rodata" prefix and once via the "a" flag.

I might easily have missed something glaring obvious, but

- I fail to see why GCC needs to be modified (and if so how):

> I think whatever the resolution we take here, you need to fix GCC.



- And what you mean/want to test with `BUILTIN` undefined? Just that SHF_ALLOC is automatically test in MC?

> You may need two RUN lines for coverage:
>
>   .ifndef BUILTIN
>   .section .rodata.cst8,"aM", at progbits,8
>   .endif
>   
>   .section .rodata.cst8


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94072/new/

https://reviews.llvm.org/D94072