Page MenuHomePhabricator

[MC][ELF] Fix accepting abbreviated form with sh_flags and sh_entsize
ClosedPublic

Authored by burnus on Jan 5 2021, 2:28 AM.

Details

Summary

Followup to D92052 as I missed an issue as shown via GCC bug https://gcc.gnu.org/PR97827, namely: (e.g.) ".rodata." implies ELF::SHF_ALLOC.

Crossref:

  • D92052 / commit 1deff4009e0ae661b03682901bf6932297ce7ea1 permitted the abbreviated form which many assemblers accept and GCC generates: while the first .section contains the flags and entsize, subsequent sections simply contain the name without repeating entsize or flags.

However, the latter patch missed in the check that some flags are automatically set, e.g. '.rodata." implies ELF::SHF_ALLOC.

Related https://bugs.llvm.org/show_bug.cgi?id=48201

Diff Detail

Event Timeline

burnus created this revision.Jan 5 2021, 2:28 AM
burnus requested review of this revision.Jan 5 2021, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2021, 2:28 AM
MaskRay added a comment.EditedJan 5 2021, 11:52 AM

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.

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

You may need two RUN lines for coverage:

.ifndef BUILTIN
.section .rodata.cst8,"aM",@progbits,8
.endif

.section .rodata.cst8
burnus added a comment.Jan 7 2021, 2:30 AM

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",@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",@progbits,8
.endif

.section .rodata.cst8

@MaskRay: See previous comment. It is not clear to me which GCC changes you think are required and what you mean by "ifndef BUILTIN" (i.e. why it is needed to test for).

I do see why a section name with ".rodata" and no flags would give different results with different assemblers, which might be unfortunate. But it is preexisting and unchanged by this patch.
I also do not see how this relates to the issue at hand, this patch, or GCC (which does outputs the SHF_ALLOC flag for the first output of a .rodata section).

Hence, from my part the attached patch is still fine – except that clang-format does not like the preexisting variable name.

I was leaving this for @MaskRay to review, since he had raised a comment.

I'm struggling to follow the initial explanation. Could you post an example of what didn't work before, and what the failure mode was, and which now works?

I'm struggling to follow the initial explanation. Could you post an example of what didn't work before, and what the failure mode was, and which now works?

As discussed in in D92052, many assemblers support that flags are only added to the first use (e.g.: .section .foo,"aM",@progbits,1) and subsequent uses omit the flag (e.g.: .section .foo).

However, for certain prefixes such as ".rodata", LLVM-MC adds default flags, namely:

if (hasPrefix(SectionName, ".rodata.") || SectionName == ".rodata1")
  Flags |= ELF::SHF_ALLOC;

Thus, the current check that for subsequent use "Flags == 0" does not as the default value is not 0 (but SHF_ALLOC).

The patch now checks for user-added flags (i.e "extraFlags == 0") instead, where "Flags" is initially the default value, "extraFlags" is what the parser returns and both are combined with "Flags |= extraFlags".

The real-world code has: (a) the .section with all flags

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

and then (b) the section without any explicit flags

.section        .rodata.cst8

but as llvm-mc adds SHF_ALLOC due to the name, there is a mismatch as the first .section has additional flags.
(Leading to the "error: changed section flags for .rodata.cst8, expected: 0x12".)

As the user did not add any flag, it makes sense to only check for the explicitly added flags (extraFlags == 0) and not for the implicitly added ones.
For code generation, only the first-used flag is used (Section->getFlags()), thus, there is no consistency problem, either.

Thanks - that explanation sounds reasonable to me. I'm not sure I really understand @MaskRay's concerns here. This seems like a pretty clear-cut bug in the previous fix.

llvm/test/MC/ELF/section-omitted-attributes.s
14

Add a comment why you have this second case.

burnus updated this revision to Diff 319549.Jan 27 2021, 6:11 AM

Change: Add comment line to test/MC/ELF/section-omitted-attributes.s

burnus marked an inline comment as done.Jan 27 2021, 6:12 AM
jhenderson accepted this revision.Jan 28 2021, 12:02 AM

LGTM, with nit.

llvm/test/MC/ELF/section-omitted-attributes.s
14

Nit: add full stop at end of comment.

This revision is now accepted and ready to land.Jan 28 2021, 12:02 AM
burnus updated this revision to Diff 319803.Jan 28 2021, 2:44 AM

(New version, only change: Add missing tailing '.')

Thanks for the review :-)

abidh added a subscriber: abidh.Jan 28 2021, 2:47 AM
This revision was landed with ongoing or failed builds.Jan 28 2021, 7:01 AM
This revision was automatically updated to reflect the committed changes.