Page MenuHomePhabricator

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

Authored by MaskRay on Feb 4 2020, 2:54 PM.

Details

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.

Diff Detail

Event Timeline

MaskRay created this revision.Feb 4 2020, 2:54 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 242437.Feb 4 2020, 2:56 PM

Use -o /dev/null

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.

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.

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.

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 planned changes to this revision.EditedFeb 5 2020, 10:02 PM

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 updated this revision to Diff 243452.EditedFeb 9 2020, 9:37 AM
MaskRay marked 4 inline comments as done.

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
MaskRay removed a subscriber: bd1976llvm.
psmith added a subscriber: psmith.Feb 10 2020, 3:44 AM

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.

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:

MaskRay updated this revision to Diff 245572.Feb 19 2020, 8:43 PM
MaskRay retitled this revision from [MC][ELF] Warn changed section type or flags to [MC][ELF] Error changed sh_type, sh_flags or sh_entsize.
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: psmith.

Change warnings to errors

MaskRay edited reviewers, added: psmith; removed: peter.smith.Feb 19 2020, 8:50 PM
MaskRay updated this revision to Diff 245573.Feb 19 2020, 8:54 PM

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 updated this revision to Diff 245580.EditedFeb 19 2020, 10:20 PM
MaskRay retitled this revision from [MC][ELF] Error changed sh_type, sh_flags or sh_entsize to [MC][ELF] Error for sh_type, sh_flags or sh_entsize change.
MaskRay edited the summary of this revision. (Show Details)

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 marked an inline comment as done.Feb 19 2020, 10:28 PM
MaskRay added inline comments.
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.

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.

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 accepted this revision.Feb 21 2020, 5:59 AM

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.

This revision is now accepted and ready to land.Feb 21 2020, 5:59 AM
MaskRay updated this revision to Diff 246025.Feb 21 2020, 3:40 PM
MaskRay edited the summary of this revision. (Show Details)

Update description

This revision was automatically updated to reflect the committed changes.

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.

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 :(

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 added a subscriber: dim.Aug 22 2020, 11:26 AM

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 added a comment.EditedAug 22 2020, 1:07 PM
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.