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

[burnus]

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:

• D73999 / commit 75af9da755721123e62b45cd0bc0c5e688a9722a added for LLVM 11 a check that sh_flags and sh_entsize (and sh_type) changes are an error, in line with GNU assembler.

• 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

[MaskRay]

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]

In 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",@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

[burnus]

@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.

[burnus]

*PING*

[burnus]

PING**2

[burnus]

*PING**3

[jhenderson]

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?

[burnus]

In D94072#2524692, @jhenderson wrote:

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.

[jhenderson]

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]

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

[jhenderson]

LGTM, with nit.

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

14

Nit: add full stop at end of comment.

[burnus]

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

Thanks for the review :-)

llvm/lib/MC/MCParser/ELFAsmParser.cpp

bool ELFAsmParser::ParseSectionArguments(bool IsPush, SMLoc loc) {
 
   if (ParseSectionName(SectionName))
     return TokError("expected identifier in directive");
 
   StringRef TypeName;
   int64_t Size = 0;
   StringRef GroupName;
   unsigned Flags = 0;
+  unsigned extraFlags = 0;
   const MCExpr *Subsection = nullptr;
   bool UseLastGroup = false;
   MCSymbolELF *LinkedToSym = nullptr;
   int64_t UniqueID = ~0;
 
   // Set the defaults first.
   if (hasPrefix(SectionName, ".rodata.") || SectionName == ".rodata1")
     Flags |= ELF::SHF_ALLOC;
if (getLexer().is(AsmToken::Comma)) {
     if (IsPush && getLexer().isNot(AsmToken::String)) {
       if (getParser().parseExpression(Subsection))
         return true;
       if (getLexer().isNot(AsmToken::Comma))
         goto EndStmt;
       Lex();
     }
 
-    unsigned extraFlags;
-
     if (getLexer().isNot(AsmToken::String)) {
       if (!getContext().getAsmInfo()->usesSunStyleELFSectionSwitchSyntax()
           || getLexer().isNot(AsmToken::Hash))
         return TokError("expected string in directive");
       extraFlags = parseSunStyleSectionFlags();
     } else {
       StringRef FlagsStr = getTok().getStringContents();
       Lex();
EndStmt:
   // for SHT_PROGBITS .eh_frame
   if (Section->getType() != Type &&
       !(SectionName == ".eh_frame" && Type == ELF::SHT_PROGBITS))
     Error(loc, "changed section type for " + SectionName + ", expected: 0x" +
                    utohexstr(Section->getType()));
   // Check that flags are used consistently. However, the GNU assembler permits
   // to leave out in subsequent uses of the same sections; for compatibility,
   // do likewise.
-  if ((Flags || Size || !TypeName.empty()) && Section->getFlags() != Flags)
+  if ((extraFlags || Size || !TypeName.empty()) && Section->getFlags() != Flags)
     Error(loc, "changed section flags for " + SectionName + ", expected: 0x" +
                    utohexstr(Section->getFlags()));
-  if ((Flags || Size || !TypeName.empty()) && Section->getEntrySize() != Size)
+  if ((extraFlags || Size || !TypeName.empty()) &&
+      Section->getEntrySize() != Size)
     Error(loc, "changed section entsize for " + SectionName +
                    ", expected: " + Twine(Section->getEntrySize()));
 
   if (getContext().getGenDwarfForAssembly() &&
       (Section->getFlags() & ELF::SHF_ALLOC) &&
       (Section->getFlags() & ELF::SHF_EXECINSTR)) {
     bool InsertResult = getContext().addGenDwarfSection(Section);
     if (InsertResult) {

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

 # RUN: llvm-mc -triple=x86_64 %s -o - | FileCheck %s
 
 # If section flags and other attributes are omitted, don't error.
 
 # CHECK: .section        .foo,"aM",@progbits,1
+# CHECK: .section        .rodata.cst8,"aM",@progbits,8
 
 .section .foo,"aM",@progbits,1
 
 .section .foo
 
 .pushsection .foo
+
+# Likewise, except that the '.rodata' prefix implies SHF_ALLOC.
+.section .rodata.cst8,"aM",@progbits,8
+
+.section .rodata.cst8