[MC][ELF] Accept abbreviated form with sh_flags and sh_entsize

[burnus]

Summary

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.

However, GNU assembler accepts and GCC generates an abbreviated form:
while the first .section contains the flags and entsize, subsequent
sections simply contain the name without repeating entsize or flags.

Do likewise for better compatibility.

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

[burnus]

Correction: The previous change (D73999) was done for "LLVM 11" - not as I wrote for "LLVM 10".

[MaskRay]

This is known. I'd like it to be an error but it is unfortunate that GCC uses it this way. Can you share a code snippet which can trigger the relevant GCC logic?

/* If we have already declared this section, we can use an
   abbreviated form to switch back to it -- unless this section is
   part of a COMDAT groups, in which case GAS requires the full
   declaration every time.  */
if (!(HAVE_COMDAT_GROUP && (flags & SECTION_LINKONCE))
    && (flags & SECTION_DECLARED))
  { 
    fprintf (asm_out_file, "\t.section\t%s\n", name);
    return;
  }

There is a variant where an empty string literal is used (https://sourceware.org/pipermail/binutils/2020-March/110441.html): I haven't receive a reply yet.
Shall we continue erroring for that?

llvm/test/MC/ELF/section-entsize-not-repeated.s

1 ↗

(On Diff #307421)

I'd rename it to section-omitted-attributes.s

llvm/test/MC/ELF/section-flags-changed-2.s

4 ↗

(On Diff #307421)

Can you merge these tests into section-entsize-changed.s?

[burnus]

In D92052#2414665, @MaskRay wrote:

This is known. I'd like it to be an error but it is unfortunate that GCC uses it this way. Can you share a code snippet which can trigger the relevant GCC logic?

It looks as if one way to get it is requires the -ffunction-sections flag, example: https://godbolt.org/z/Yfveoh which creates:

• .section .text.ULtod,"ax",@progbits
• .section .text.ULtod

-ffunction-sections/-fdata-sections is described as: Place each function or data item into its own section in the output file if the target supports arbitrary sections. ... Only use these options when there are significant benefits from doing so. When you specify these options, the assembler and linker create larger object and executable files and are also slower.

As there is already a huge growth, avoiding the repetition of the flags/entsize helps a bit. Thus, the GCC and GNU assembler short variant makes kind of sense.

There is a variant where an empty string literal is used (https://sourceware.org/pipermail/binutils/2020-March/110441.html): I haven't receive a reply yet.
Shall we continue erroring for that?

No strong opinion, but I don't see how why one would write such code in real-world code; and if there is no good reason for valid code, giving an error should not harm, should it?

[MaskRay]

In D92052#2414890, @burnus wrote:

In D92052#2414665, @MaskRay wrote:

This is known. I'd like it to be an error but it is unfortunate that GCC uses it this way. Can you share a code snippet which can trigger the relevant GCC logic?

It looks as if one way to get it is requires the -ffunction-sections flag, example: https://godbolt.org/z/Yfveoh which creates:

• .section .text.ULtod,"ax",@progbits
• .section .text.ULtod

-ffunction-sections/-fdata-sections is described as: Place each function or data item into its own section in the output file if the target supports arbitrary sections. ... Only use these options when there are significant benefits from doing so. When you specify these options, the assembler and linker create larger object and executable files and are also slower.

As there is already a huge growth, avoiding the repetition of the flags/entsize helps a bit. Thus, the GCC and GNU assembler short variant makes kind of sense.

OK, thanks for the example: a jump table causes the second .section .text.foobar directive without attributes. This makes sense.

There is a variant where an empty string literal is used (https://sourceware.org/pipermail/binutils/2020-March/110441.html): I haven't receive a reply yet.
Shall we continue erroring for that?

No strong opinion, but I don't see how why one would write such code in real-world code; and if there is no good reason for valid code, giving an error should not harm, should it?

Right, if we can continue rejecting .section .foo,"" relatively easily, I hope we continue rejecting it (even though GNU as allows it). If a bit difficult, I'll not push too hard on losing the diagnostic.

llvm/test/MC/ELF/section-flags-changed-2.s

9 ↗

(On Diff #307421)

Please drop tests which are already tested in section-flags-changed.s

[burnus]

Address review comments.

I also added the example from https://sourceware.org/pipermail/binutils/2020-March/110441.html for which the previous patch gave (as with GNU as) no error:
.section .foo,"",@progbits; .byte 2

I fixed that example by using: (Flags || Size || !TypeName.empty()) &&. However, to handle even more, some boolean flag needs to be set during the parsing, which I didn't do (and seems to be overly complex just for diagnostics).

[jhenderson]

llvm/test/MC/ELF/section-entsize-changed.s

14 ↗

(On Diff #307520)

Nit: delete extra blank line.

[burnus]

Fix review nit – two instead of one empty line inside a test case.

[MaskRay]

llvm/test/MC/ELF/section-entsize-changed.s

14 ↗

(On Diff #307556)

Unused(should be deleted)

28 ↗

(On Diff #307556)

.byte directives are unused (should be deleted)

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

4

#

12

No need for multiple pushsection .foo

[burnus]

Fix review comments (all in the testcases)

Thanks for the reviews!

[burnus]

Review PING – Is there anything else to be changed?

[burnus]

REVIEW PING

I think all review comments by @jhenderson and @MaskRay have been fixed last week (thanks for the good suggestions!).
However, a (final) review is still pending ... Thus, if have time, please suggest additional fixes (or approve). Thanks!

[MaskRay]

llvm/test/MC/ELF/section-entsize-changed.s

14 ↗

(On Diff #307637)

The labels are unused. Please remove them.

18 ↗

(On Diff #307637)

Why does the additional test duplicate the testing of changed flags which has been covered by section-flags-changed.s?

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

14

.bar just duplicates .foo , please drop .bar

And also foo

[burnus]

@MaskRay: Thanks for the review. I hope it now makes sense.

As only '.section .foobar,"",@progbits`' is kind of a new check ("" was not tested before), I kept it but added it to section-flags-changed.s instead of section-entsize-changed.s. In section-omitted-attributes.s, I removed the only slightly different variant.

[MaskRay]

Please wait for @jhenderson

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

2

Please add a file-level comment about the purpose of the test:

If section flags and other attributes are omitted, don't error.

[jhenderson]

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

4

Is it worth adding something like this after this line:

CHECK-NOT: .section
CHECK-NOT: .pushsection

? I don't know if it's actually what happens, but it wouldn't make sense to repeat the section directives if they aren't needed.

[burnus]

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

4

Is it worth adding something like this after this line:

CHECK-NOT: .section
CHECK-NOT: .pushsection

? I don't know if it's actually what happens, but it wouldn't make sense to repeat the section directives if they aren't needed.

What happens with llvm-mc -triple=x86_64 -o - is (full output without empty lines):

.text
.section        .foo,"aM",@progbits,1

Thus, all .foo sections are merged and the originally specified flag/entsize is used.

We could add the CHECK-NOT but my feeling is that it is needed.

In the real-world code, the '.section' are repeated (with GCC) with -ffunction-section as the example https://godbolt.org/ shows – as described above in my previous comment ( https://reviews.llvm.org/D92052#2414890 ).

To quote @MaskRay: "a jump table causes the second .section .text.foobar directive without attributes. This makes sense."

[burnus]

@jhenderson – did I answer your questions - or did I misread them? Are the other issues?

[jhenderson]

In D92052#2448057, @burnus wrote:

@jhenderson – did I answer your questions - or did I misread them? Are the other issues?

Apologies, I somehow missed this. LGTM once @MaskRay's outstanding comment has been addressed.

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

2

It looks like @MaskRay's comment hasn't been addressed here?

[burnus]

Thanks for the reviews! Only change: added proposed comment string to llvm/test/MC/ELF/section-omitted-attributes.s regarding what the patch does.

[abidh]

Committed in 1deff4009e0a on request of @burnus.

[burnus]

As it is a regression and affects real-world code (https://bugs.llvm.org/show_bug.cgi?id=48201),
I like to backport it to LLVM 11. – Any reasons not to do so?

@tstellar Can this still go into LLVM 11.0.1? It is a very small fix for a regression and I see that https://bugs.llvm.org/show_bug.cgi?id=release-11.0.1
still lists 18 open bugs.

[jhenderson]

In D92052#2449063, @burnus wrote:

As it is a regression and affects real-world code (https://bugs.llvm.org/show_bug.cgi?id=48201),
I like to backport it to LLVM 11. – Any reasons not to do so?

@tstellar Can this still go into LLVM 11.0.1? It is a very small fix for a regression and I see that https://bugs.llvm.org/show_bug.cgi?id=release-11.0.1
still lists 18 open bugs.

You may also want to try pinging him on the bug itself.

[tstellar]

@jhenderson What's your opinion on backporting this?

[jhenderson]

In D92052#2451933, @tstellar wrote:

@jhenderson What's your opinion on backporting this?

I think it's reasonably safe, because the change just means fewer errors reported by the assembler, and it's a fix for effectively a regression, so we're just going back to older behaviour. However, I'm not the most up-to-speed with the surrounding code, so @MaskRay may be a better person to give their opinion.

[MaskRay]

In D92052#2452038, @jhenderson wrote:

In D92052#2451933, @tstellar wrote:

@jhenderson What's your opinion on backporting this?

I think it's reasonably safe, because the change just means fewer errors reported by the assembler, and it's a fix for effectively a regression, so we're just going back to older behaviour. However, I'm not the most up-to-speed with the surrounding code, so @MaskRay may be a better person to give their opinion.

@tstellar This makes an integrated assembler error less strict, so it is safe.

This is about assembling GCC's -S output with LLVM's integrated assembler, which isn't an area that I think people frequently use, so I'd also hear from @burnus why this is urgent.

[burnus]

In D92052#2452442, @MaskRay wrote:

This is about assembling GCC's -S output with LLVM's integrated assembler, which isn't an area that I think people frequently use, so I'd also hear from @burnus why this is urgent.

GCC's AMD GCN (alias Radeon) target (most useful for offloading) uses LLVM's assembler as GNU Binutils does not support GCN (yet?).

Many Linux distributions build GCC with offloading support for nvptx and amd gcn (installable was optional add-on packages).

The issue was found as building GCC was breaking on Debian once LLVM 11.0.0 was imported into Debian unstable. (Happens during the build of newlib's libc for GCN; this uses -ffunction-section). First reported at https://gcc.gnu.org/PR97827

[tstellar]

In D92052#2452577, @burnus wrote:

In D92052#2452442, @MaskRay wrote:

This is about assembling GCC's -S output with LLVM's integrated assembler, which isn't an area that I think people frequently use, so I'd also hear from @burnus why this is urgent.

GCC's AMD GCN (alias Radeon) target (most useful for offloading) uses LLVM's assembler as GNU Binutils does not support GCN (yet?).

Many Linux distributions build GCC with offloading support for nvptx and amd gcn (installable was optional add-on packages).

The issue was found as building GCC was breaking on Debian once LLVM 11.0.0 was imported into Debian unstable. (Happens during the build of newlib's libc for GCN; this uses -ffunction-section). First reported at https://gcc.gnu.org/PR97827

Have any other targets in gcc besides amdgcn run into this problem? If this is something specific to amdgcn, then I'm not sure we should be adding a workaround for it to LLVM.

[burnus]

In D92052#2452601, @tstellar wrote:

Have any other targets in gcc besides amdgcn run into this problem? If this is something specific to amdgcn, then I'm not sure we should be adding a workaround for it to LLVM.

The issue is general – but for AMD GCN it shows up first as this one always uses LLVM's linker.

It is also not really a "workaround" for a bug. GCC on purpose does not repeat the flags for .section in order to reduce the huge assembler file size one gets with -ffunction-sections.

[MaskRay]

@burnus Looks like you did not mention https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97827 in the initial review. From the linked thread
https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559572.html

Richard Sandiford's comment makes a lot of sense to me: "(a) doing something that works “everywhere” unconditionally (and keeping things simple) vs. (b) having both code that takes a shortcut and code that doesn't take a shortcut and trying to predict which one we should do."

I am now feeling a bit bad that reviewers did not receive full discussions before making a decision to accept the fix or workaround (depending on how the GCC community reacts). If they do want to fix 11, we might not need this patch or we could say this was for GCC before 11, making a better description.

[burnus]

In D92052#2452633, @MaskRay wrote:

@burnus Looks like you did not mention https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97827 in the initial review.

The link is in https://bugs.llvm.org/show_bug.cgi?id=48201 from the very beginning, and that bug was linked in here.

[MaskRay]

In D92052#2452662, @burnus wrote:

In D92052#2452633, @MaskRay wrote:

@burnus Looks like you did not mention https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97827 in the initial review.

The link is in https://bugs.llvm.org/show_bug.cgi?id=48201 from the very beginning, and that bug was linked in here.

OK my bad (I don't regularly see "See Also"... ) I've sent https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97827#c9 . I'll follow up with GCC folks there. I don't subscribe gcc-patches so it will be a bit inconvenient for me if most people comment on the mailing list instead...

[jakubjelinek]

In D92052#2452686, @MaskRay wrote:

OK my bad (I don't regularly see "See Also"... ) I've sent https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97827#c9 . I'll follow up with GCC folks there. I don't subscribe gcc-patches so it will be a bit inconvenient for me if most people comment on the mailing list instead...

There is nothing GNU as specific on it, all assemblers I'm aware of behave like that.
See e.g. https://docs.huihoo.com/solaris/2.4/801-6649/801-6649.pdf
"If section_name is a non-reserved section, attributes must be included the first time it is specified by the.section directive."
you can search for others. It isn't solely about what GCC emits either, there is a lot of hand-written assembler code in the wild and that also uses the documented .section directive behavior.
I'm strongly against changing what GCC emits here.

[MaskRay]

In D92052#2452791, @jakubjelinek wrote:

In D92052#2452686, @MaskRay wrote:

OK my bad (I don't regularly see "See Also"... ) I've sent https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97827#c9 . I'll follow up with GCC folks there. I don't subscribe gcc-patches so it will be a bit inconvenient for me if most people comment on the mailing list instead...

There is nothing GNU as specific on it, all assemblers I'm aware of behave like that.
See e.g. https://docs.huihoo.com/solaris/2.4/801-6649/801-6649.pdf
"If section_name is a non-reserved section, attributes must be included the first time it is specified by the.section directive."
you can search for others. It isn't solely about what GCC emits either, there is a lot of hand-written assembler code in the wild and that also uses the documented .section directive behavior.
I'm strongly against changing what GCC emits here.

Thanks for chiming in :) OK, I am happy to keep this logic.

@tstellar It should be a good candidate for backporting.

llvm/lib/MC/MCParser/ELFAsmParser.cpp

EndStmt:
   getStreamer().SwitchSection(Section, Subsection);
   // x86-64 psABI names SHT_X86_64_UNWIND as the canonical type for .eh_frame,
   // but GNU as emits SHT_PROGBITS .eh_frame for .cfi_* directives. Don't error
   // 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()));
-  if (Section->getFlags() != Flags)
+  // 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)
     Error(loc, "changed section flags for " + SectionName + ", expected: 0x" +
                    utohexstr(Section->getFlags()));
-  if (Section->getEntrySize() != Size)
+  if ((Flags || 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-flags-changed.s

 # RUN: not llvm-mc -triple=x86_64 %s -o /dev/null 2>&1 | FileCheck %s --implicit-check-not=error:
 
 foo:
 .section .foo,"ax",@progbits
 
 # CHECK: {{.*}}.s:[[# @LINE+1]]:1: error: changed section flags for .foo, expected: 0x6
 .section .foo,"awx",@progbits
 
 # CHECK: {{.*}}.s:[[# @LINE+1]]:1: error: changed section flags for .foo, expected: 0x6
 .pushsection .foo,"a",@progbits
 
+# CHECK: {{.*}}.s:[[# @LINE+1]]:1: error: changed section flags for .foo, expected: 0x6
+.section .foo,"",@progbits
+
 .pushsection .foo,"ax",@progbits

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
+
+.section .foo,"aM",@progbits,1
+
+.section .foo
+
+.pushsection .foo