[MC][ELF] Error for sh_type, sh_flags or sh_entsize change

[MaskRay]

Summary

Heads-up message: https://lists.llvm.org/pipermail/llvm-dev/2020-February/139390.html

GNU as started to emit warnings for changed sh_type or sh_flags in 2000.
GNU as>=2.35 will emit errors for most type/flags change, and error for entsize change.

Some cases () remain warnings for legacy reasons:

.section .init_array,"ax", at progbits
.section .init_array,"ax", at init_array
# And some obscure sh_flags changes (OS/Processor specific flags)

The rationale of a diagnostic (warning or error) is that sh_type,
sh_flags or sh_entsize changes usually indicate user errors.

We just try to be more rigid and emit errors for all sh_type/sh_flags/sh_entsize change.

A possible improvement in the future is to reuse
llvm-readobj/ELFDumper.cpp:getSectionTypeString so that we can name the
type in the diagnostics.

[MaskRay]

Use -o /dev/null

[MaskRay]

There is a discussion on https://bugs.llvm.org/show_bug.cgi?id=44775 about whether the following two directives should mean the different sections

.data
foo:
bar:
.section .text,"axo",%progbits,foo
.section .text,"axo",%progbits,bar

Perhaps you can chime in.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[jhenderson]

The warnings seem okay for now, though I do wonder whether really all the properties of a section should just be used in uniquely identifying a section.

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

11

Any reason you've tried to change the flags twice?

13

Possibly worth a .pushsection case which changes the flags?

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

10

Any particular reason for this extra case here?

[MaskRay]

Postpone this patch. We need to wait for the resolution to "what fields are used to differentiate two sections"

Currently it is (section_name,group_name,unique_id).
Adding linked_to_sym seems reasonable (D74006; it is hjl's GNU as patch will do). hjl and I favor it.
One opinion (@bd1976llvm's) is that sh_entsize should also be taken into account.
If we eventually add sh_type/sh_flags to the differentiator, then this patch will not be needed.

There are some discussions on D74006 and https://bugs.llvm.org/show_bug.cgi?id=44775

edit: Created https://sourceware.org/ml/binutils/2020-02/msg00091.html

[MaskRay]

Address review comments.

I tend to agree with Alan Modra's arguement in
https://sourceware.org/ml/binutils/2020-02/msg00093.html

I don't think so. User assembly often gets section attributes wrong
or leaves them off entirely for special sections known to the
assembler. ie. the first .section .foo above is a built-in rather
than user input.

Add more folks for feedback.

Please see my binutils post. For a .section directive with the same name but a different field. If the field is:

• sh_flags or sh_type: warn
• sh_link due to SHF_LINK_ORDER: no warning. produce separate sections
• sh_entsize due to SHF_MERGE: still controversial

[psmith]

In D73999#1866168, @MaskRay wrote:

Address review comments.

I tend to agree with Alan Modra's arguement in
https://sourceware.org/ml/binutils/2020-02/msg00093.html

I don't think so. User assembly often gets section attributes wrong
or leaves them off entirely for special sections known to the
assembler. ie. the first .section .foo above is a built-in rather
than user input.

Add more folks for feedback.

Please see my binutils post. For a .section directive with the same name but a different field. If the field is:

• sh_flags or sh_type: warn
• sh_link due to SHF_LINK_ORDER: no warning. produce separate sections
• sh_entsize due to SHF_MERGE: still controversial

I agree with warning for sh_flags, I think there is too much legacy code out there that would behave differently.
I agree with sh_link producing separate sections. In an ideal world no-one writes this by hand in assembly.
For sh_entsize SHF_MERGE, this is informing the linker that it can produce an optimisation, which it is permitted to ignore. I tend towards an error message if it is implementable as it is a strong message to fix the assembler. The next best thing is a warning then clearing SHF_MERGE and setting sh_entsize to 0. I don't think a warning on its own is safe.

[bd1976llvm]

In D73999#1866680, @psmith wrote:

In D73999#1866168, @MaskRay wrote:

Address review comments.

I tend to agree with Alan Modra's arguement in
https://sourceware.org/ml/binutils/2020-02/msg00093.html

I don't think so. User assembly often gets section attributes wrong
or leaves them off entirely for special sections known to the
assembler. ie. the first .section .foo above is a built-in rather
than user input.

Add more folks for feedback.

Please see my binutils post. For a .section directive with the same name but a different field. If the field is:

• sh_flags or sh_type: warn
• sh_link due to SHF_LINK_ORDER: no warning. produce separate sections
• sh_entsize due to SHF_MERGE: still controversial

I agree with warning for sh_flags, I think there is too much legacy code out there that would behave differently.
I agree with sh_link producing separate sections. In an ideal world no-one writes this by hand in assembly.
For sh_entsize SHF_MERGE, this is informing the linker that it can produce an optimisation, which it is permitted to ignore. I tend towards an error message if it is implementable as it is a strong message to fix the assembler. The next best thing is a warning then clearing SHF_MERGE and setting sh_entsize to 0. I don't think a warning on its own is safe.

After @MaskRay's discussion with the GNU binutils people I think the following will work for sh_entsize:

• In the assembler emit an error (as long as https://sourceware.org/ml/binutils/2020-02/msg00129.html goes through) for two section directives with the same section name but incompatible sh_entsize values.
• When lowering symbols "normally" there isn't a problem as we just bin compatible symbols into differently named sections e.g. .rodata.cst4 or .rodata.str4.4.
• When lowering symbols that are explicitly given a section name we will need a fix for https://bugs.llvm.org/show_bug.cgi?id=43457, see also comments in https://reviews.llvm.org/D72194.

[MaskRay]

Change warnings to errors

[MaskRay]

Improve diagnostics for changed sh_entsize.
Add section-entsize-changed.s

There are currently no good way to provide better diagnostics for sh_type and sh_flags

[MaskRay]

Improve diagnostics

GNU as started to emit warnings in 2000
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=742f45cf3ae24b4dd4bd63ddcce18bddf1f31330

GNU as 2.35 will emit errors for most cases
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=33176d912add7680277ad5e18af0e6303d9a7af8

I think it is reasonable for us to start with errors. If there are justified complaints for sh_type, we can consider downgrading to warnings. sh_flags/sh_entsize changes are very likely real errors.

[MaskRay]

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

7

0x6 is not ideal in terms of UI friendliness, but I think it is a fine compromise.
Additional complexity is that some sh_flags bits are OS/Processor specific, so the mapping isn't 1:1.

This diagnostic targets assembly users. They know how to figure out what they should write for the .section directive.

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

10

A very weak reason for checking a different type. I think the reason is obvious from the test case name and the warning, so I will delete it.

[psmith]

I'm a little surprised at different section attributes being considered a warning given previous comments that users often get this wrong, but the GNU as commit does indeed error so I think it will be better to be consistent. I tend towards putting in the effort to get a better error message, yes experienced assembler writers will know what to do, but these people aren't always the ones looking at build failures from CI. With some extra context we could save quite a lot of people a little bit of time. One possible approach would be to see what the impact is on the projects that build from tip of trunk or close to it and proceed according to feeedback.

This may be worth a heads up message to the list so that people have some warning that some of their projects may start failing to build due to errors in assembly that they've got away with up to now.

[MaskRay]

In D73999#1885011, @psmith wrote:

I'm a little surprised at different section attributes being considered a warning given previous comments that users often get this wrong, but the GNU as commit does indeed error so I think it will be better to be consistent. I tend towards putting in the effort to get a better error message, yes experienced assembler writers will know what to do, but these people aren't always the ones looking at build failures from CI. With some extra context we could save quite a lot of people a little bit of time. One possible approach would be to see what the impact is on the projects that build from tip of trunk or close to it and proceed according to feeedback.

This may be worth a heads up message to the list so that people have some warning that some of their projects may start failing to build due to errors in assembly that they've got away with up to now.

Thanks for the suggestion. Made a heads-up on the mailing list https://lists.llvm.org/pipermail/llvm-dev/2020-February/139390.html

[psmith]

Thanks for sending the heads up message. I see no-one has objected (yet), I've marked as approved as I think it is worth going ahead. Will be worth giving some time, end of today perhaps, for others to respond before committing.

[MaskRay]

Update description

[stella.stamenova]

Looks like this broke the Windows LLDB bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/14056

[MaskRay]

In D73999#1887767, @stella.stamenova wrote:

Looks like this broke the Windows LLDB bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/14056

The tests used wrong sh_flags caught by this assembler change.

Hopefully fixed by ebee131259719fa9c06cd346e21ace3fa8ac0888. I made a typo in my description. sh_flags was incorrect, not sh_type.

[stella.stamenova]

In D73999#1887784, @MaskRay wrote:

In D73999#1887767, @stella.stamenova wrote:

Looks like this broke the Windows LLDB bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/14056

The tests used wrong sh_flags caught by this assembler change.

Hopefully fixed by ebee131259719fa9c06cd346e21ace3fa8ac0888. I made a typo in my description. sh_flags was incorrect, not sh_type.

Thanks! It looks like one of them is still broken though :(

[MaskRay]

In D73999#1887836, @stella.stamenova wrote:

In D73999#1887784, @MaskRay wrote:

In D73999#1887767, @stella.stamenova wrote:

Looks like this broke the Windows LLDB bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/14056

The tests used wrong sh_flags caught by this assembler change.

Hopefully fixed by ebee131259719fa9c06cd346e21ace3fa8ac0888. I made a typo in my description. sh_flags was incorrect, not sh_type.

Thanks! It looks like one of them is still broken though :(

Fixed by e29065a105342a904871437d18a4e6fab09e5bc1.

Sorry for the churn.

[dim]

After this change, it turns out that some of the errors that this results in can be very confusing. For example, when building ocaml, it uses a .S file (https://github.com/ocaml/ocaml/blob/trunk/runtime/amd64.S#L729) containing:

.section .rodata.cst8,"a",@progbits

Note that this is the first time in the file any such section is mentioned! Then clang 11 gives an error which appears to indicate that the flags and entsize have "changed", even though the user has no idea that these sections apparently have built-in defaults:

amd64.S:1:1: error: changed section flags for .rodata.cst8, expected: 0x12
.section .rodata.cst8,"a",@progbits
^
amd64.S:1:1: error: changed section entsize for .rodata.cst8, expected: 8
.section .rodata.cst8,"a",@progbits
^

Apparently the flags must be "aM"` instead, but that is hard to tell from the error message. Also, the directive does not specify an entsize at all, so how can it then complain that changed? That latter error message is not only misleading, but simply incorrect.

If you change the flags to "aM", the complaint about the 'changed' flags goes away, but now you get another error about entsize:

md64.S:1:37: error: expected the entry size
.section .rodata.cst8,"aM",@progbits
                                    ^

So now it suddenly expects that size, while it previously fetched some internal default from somewhere?

Note that GNU as (I used 2.33.1) has much less trouble with all this:

• If you assemble the first example, e.g. .section .rodata.cst8,"a",@progbits, it will not warn at all.
• If you assemble the second example, e.g. .section .rodata.cst8,"aM",@progbits, it will warn: amd64.S:1: Warning: entity size for SHF_MERGE not specified. Which is much nicer, and does not break the build.

[MaskRay]

In D73999#2232111, @dim wrote:

After this change, it turns out that some of the errors that this results in can be very confusing. For example, when building ocaml, it uses a .S file (https://github.com/ocaml/ocaml/blob/trunk/runtime/amd64.S#L729) containing:

.section .rodata.cst8,"a",@progbits

Note that this is the first time in the file any such section is mentioned! Then clang 11 gives an error which appears to indicate that the flags and entsize have "changed", even though the user has no idea that these sections apparently have built-in defaults:

amd64.S:1:1: error: changed section flags for .rodata.cst8, expected: 0x12
.section .rodata.cst8,"a",@progbits
^
amd64.S:1:1: error: changed section entsize for .rodata.cst8, expected: 8
.section .rodata.cst8,"a",@progbits
^

Apparently the flags must be "aM"` instead, but that is hard to tell from the error message. Also, the directive does not specify an entsize at all, so how can it then complain that changed? That latter error message is not only misleading, but simply incorrect.

If you change the flags to "aM", the complaint about the 'changed' flags goes away, but now you get another error about entsize:

md64.S:1:37: error: expected the entry size
.section .rodata.cst8,"aM",@progbits
                                    ^

So now it suddenly expects that size, while it previously fetched some internal default from somewhere?

Note that GNU as (I used 2.33.1) has much less trouble with all this:

• If you assemble the first example, e.g. .section .rodata.cst8,"a",@progbits, it will not warn at all.
• If you assemble the second example, e.g. .section .rodata.cst8,"aM",@progbits, it will warn: amd64.S:1: Warning: entity size for SHF_MERGE not specified. Which is much nicer, and does not break the build.

GNU as knows that .rodata.* needs to be non-SHF_WRITE. It warns ignoring changed section attributes for .rodata.cst8 if the section flag w is set. GNU as 2.35 changed some warnings to errors. However, it does appear to still ignore M changes. The rule it uses is quite complex (gas/config/obj-elf.c:obj_elf_change_section).

LLVM MC is less permissive. In this case I think an error is the intended behavior, otherwise the section may collide with a compiler-generated .rodata.cst8 which has the SHF_MERGE flag (you can use the .section directive in inline assembly). I can be argued that when assembling a .s, MC can be taught not to treat built-in sections. This would require larger changes to MC. I tend to think ocaml needs to be patched in this case. It might cause some frictions as the user has to know the special sections and refrain from making different type/flag decisions.

llvm/lib/MC/MCParser/ELFAsmParser.cpp

if (UseLastGroup) {
     if (const MCSectionELF *Section =
             cast_or_null<MCSectionELF>(CurrentSection.first))
       if (const MCSymbol *Group = Section->getGroup()) {
         GroupName = Group->getName();
         Flags |= ELF::SHF_GROUP;
       }
   }
 
-  MCSection *ELFSection = getContext().getELFSection(
+  MCSectionELF *Section = getContext().getELFSection(
       SectionName, Type, Flags, Size, GroupName, UniqueID, LinkedToSym);
-  getStreamer().SwitchSection(ELFSection, Subsection);
+  getStreamer().SwitchSection(Section, Subsection);
+  if (Section->getType() != Type)
+    Error(loc, "changed section type for " + SectionName + ", expected: 0x" +
+                   utohexstr(Section->getType()));
+  if (Section->getFlags() != Flags)
+    Error(loc, "changed section flags for " + SectionName + ", expected: 0x" +
+                   utohexstr(Section->getFlags()));
+  if (Section->getEntrySize() != Size)
+    Error(loc, "changed section entsize for " + SectionName +
+                   ", expected: " + Twine(Section->getEntrySize()));
 
   if (getContext().getGenDwarfForAssembly()) {
-    bool InsertResult = getContext().addGenDwarfSection(ELFSection);
+    bool InsertResult = getContext().addGenDwarfSection(Section);
     if (InsertResult) {
       if (getContext().getDwarfVersion() <= 2)
         Warning(loc, "DWARF2 only supports one section per compilation unit");
 
-      if (!ELFSection->getBeginSymbol()) {
+      if (!Section->getBeginSymbol()) {
         MCSymbol *SectionStartSymbol = getContext().createTempSymbol();
         getStreamer().emitLabel(SectionStartSymbol);
-        ELFSection->setBeginSymbol(SectionStartSymbol);
+        Section->setBeginSymbol(SectionStartSymbol);
       }
     }
   }
 
   return false;
 }
 
 bool ELFAsmParser::ParseDirectivePrevious(StringRef DirName, SMLoc) {

llvm/test/MC/ELF/exclude-debug-dwo.s

 # RUN: llvm-mc -triple x86_64-pc-linux-gnu %s -filetype=obj -o %t.o
 # RUN: llvm-readelf -S %t.o | FileCheck %s
 
 ## Check we add SHF_EXCLUDE flag for .dwo debug sections.
 # CHECK: .debug_info.dwo        {{.*}} E
 # CHECK: .debug_types.dwo       {{.*}} E
 # CHECK: .debug_abbrev.dwo      {{.*}} E
 # CHECK: .debug_str.dwo         {{.*}} MSE
 # CHECK: .debug_line.dwo        {{.*}} E
 # CHECK: .debug_loc.dwo         {{.*}} E
 # CHECK: .debug_str_offsets.dwo {{.*}} E
 
-.section .debug_info.dwo
+.section .debug_info.dwo,"e"
 nop
 
-.section .debug_types.dwo
+.section .debug_types.dwo,"e"
 nop
 
-.section .debug_abbrev.dwo
+.section .debug_abbrev.dwo,"e"
 nop
 
-.section .debug_str.dwo
+.section .debug_str.dwo,"MSe",@progbits,1
 nop
 
-.section .debug_line.dwo
+.section .debug_line.dwo,"e"
 nop
 
-.section .debug_loc.dwo
+.section .debug_loc.dwo,"e"
 nop
 
-.section .debug_str_offsets.dwo
+.section .debug_str_offsets.dwo,"e"
 nop

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

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

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

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

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