This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Use SHF_SUNW_NODISCARD instead of SHF_GNU_RETAIN on Solaris
ClosedPublic

Authored by ro on Aug 12 2021, 4:41 AM.

Details

Summary

Instead of the GNU extension SHF_GNU_RETAIN, Solaris provides equivalent
functionality with SHF_SUNW_NODISCARD. This patch implements the necessary
support.

Tested on sparcv9-sun-solaris2.11, amd64-pc-solaris2.11, and x86_64-pc-linux-gnu.

The patch passes existing tests, although I suspect something might still be missing.
I've inspected elf-retain.o with both Solaris' native elfdump and llvm-readelf to
check that the SHF_SUNW_NODISCARD flag is indeed present.

One caveat is that, unlike Solaris, Illumos and derivatives don't support
SHF_SUNW_NODISCARD. Unfortunately, there's currently no way to distinguish
the two related OSes and the Illumos community barely reacted to my attempts to
reach out to them.

Diff Detail

Event Timeline

ro created this revision.Aug 12 2021, 4:41 AM
ro requested review of this revision.Aug 12 2021, 4:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2021, 4:41 AM
MaskRay added a comment.EditedAug 12 2021, 4:16 PM

The convention is to add lib/Object, llvm-readobj, yaml2obj, obj2yaml changes (dumper and yaml2obj) in one patch. Example: D107949. Also see needed tests there.

MC and CodeGen/TargetLoweringObjectFileImpl.cpp are less close to the above files and should be placed in a separate patch.

One caveat is that, unlike Solaris, Illumos and derivatives don't support SHF_SUNW_NODISCARD. Unfortunately, there's currently no way to distinguish the two related OSes and the Illumos community barely reacted to my attempts to reach out to them.

The ELF spirit is to ignore what the implementation don't recognize. A redundant unrecognized section flag is usually fine.

If there is a potential Illumos compatibility problem, it's clearly on their side and you are not expected to do more.

MaskRay requested changes to this revision.Jan 26 2022, 11:48 AM

Ping for more tests:)

Request changes so that this is out of others' and my review queue.

This revision now requires changes to proceed.Jan 26 2022, 11:48 AM

I've finally gotten around to this: MC and CodeGen parts split off as D120318.

ro updated this revision to Diff 410504.Feb 22 2022, 4:07 AM
  • Split off MC and CodeGen parts as D120318.
  • Add testcases.
MaskRay added inline comments.Feb 22 2022, 10:24 AM
llvm/test/tools/llvm-readobj/ELF/section-flags-solaris.test
29

align:

- Name:    .os.flags.low
  Type:    SHT_PROGBITS
  ShFlags: 0x00100000
llvm/test/tools/yaml2obj/ELF/retain-section.yaml
20

remove excessive padding while keep aligning.

llvm/tools/llvm-readobj/ELFDumper.cpp
1249

Fix clang-format lints

ro marked 2 inline comments as done.Feb 22 2022, 12:54 PM
ro added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
1249

I'd rather not: clang-format-diff.py makes a total mess here: with LLVM 14 clang-format, the block for the gABI flags is changed like this:

 const EnumEntry<unsigned> ElfSectionFlags[] = {
-  ENUM_ENT(SHF_WRITE,            "W"),
-  ENUM_ENT(SHF_ALLOC,            "A"),
-  ENUM_ENT(SHF_EXECINSTR,        "X"),
-  ENUM_ENT(SHF_MERGE,            "M"),
-  ENUM_ENT(SHF_STRINGS,          "S"),
-  ENUM_ENT(SHF_INFO_LINK,        "I"),
-  ENUM_ENT(SHF_LINK_ORDER,       "L"),
-  ENUM_ENT(SHF_OS_NONCONFORMING, "O"),
-  ENUM_ENT(SHF_GROUP,            "G"),
-  ENUM_ENT(SHF_TLS,              "T"),
-  ENUM_ENT(SHF_COMPRESSED,       "C"),
-  ENUM_ENT(SHF_EXCLUDE,          "E"),
+    ENUM_ENT(SHF_WRITE, "W"),      ENUM_ENT(SHF_ALLOC, "A"),
+    ENUM_ENT(SHF_EXECINSTR, "X"),  ENUM_ENT(SHF_MERGE, "M"),
+    ENUM_ENT(SHF_STRINGS, "S"),    ENUM_ENT(SHF_INFO_LINK, "I"),
+    ENUM_ENT(SHF_LINK_ORDER, "L"), ENUM_ENT(SHF_OS_NONCONFORMING, "O"),
+    ENUM_ENT(SHF_GROUP, "G"),      ENUM_ENT(SHF_TLS, "T"),
+    ENUM_ENT(SHF_COMPRESSED, "C"), ENUM_ENT(SHF_EXCLUDE, "E"),
 };

and the next two indented further, making them inconsistent with the following ones.

ro updated this revision to Diff 410619.Feb 22 2022, 12:58 PM

Whitespace fixes.

MaskRay accepted this revision.Feb 22 2022, 1:19 PM

LGTM. @jhenderson may have further opinions.

This revision is now accepted and ready to land.Feb 22 2022, 1:19 PM
jhenderson added inline comments.Feb 23 2022, 12:45 AM
llvm/test/tools/yaml2obj/ELF/retain-section.yaml
2

It might be interesting to show what happens if you use SHF_GNU_RETAIN on Solaris and SHF_SUNW_NODISCARD on non-Solaris.

llvm/tools/llvm-readobj/ELFDumper.cpp
1249

You should be able to format the new enum entry lists at least though.

ro marked an inline comment as done.Feb 23 2022, 1:37 AM
ro added inline comments.
llvm/test/tools/yaml2obj/ELF/retain-section.yaml
2

Excellent point: the first silently creates what Solaris elfdump identifies as SHF_SUNW_ABSENT, but should be rejected. The second at least errors out:

YAML:68:14: error: unknown bit value
    Flags: [ SHF_SUNW_NODISCARD ]
             ^~~~~~~~~~~~~~~~~~~

although the error message could be clearer.

llvm/tools/llvm-readobj/ELFDumper.cpp
1249

Right, fixed. Indentation is still confusing, though: aligned for e.g. ElfSectionFlags, just one space for the single-entry arrays. Oh well.

ro updated this revision to Diff 410746.Feb 23 2022, 2:20 AM
ro marked an inline comment as done.
  • Formatting fixes.
  • Improve/test error handling.
jhenderson added inline comments.Feb 23 2022, 4:48 AM
llvm/test/tools/yaml2obj/ELF/retain-section.yaml
47

It would be worth including the context saying which flag is the unknown value.

63

Ditto. Also add a space to match the earlier veresion.

74

Nit: too many new lines at EOF (need exactly one).

ro marked an inline comment as done.Feb 23 2022, 5:24 AM
ro added inline comments.
llvm/test/tools/yaml2obj/ELF/retain-section.yaml
47

Good point indeed. I've indented the Flags: part, matching what yaml2obj does. Is this ok or is the output supposed to be aligned in the testcase?

ro updated this revision to Diff 410777.Feb 23 2022, 5:25 AM
  • Whitespace fixes.
  • Tighten error checking.
jhenderson accepted this revision.Feb 23 2022, 6:03 AM

LGTM.

llvm/test/tools/yaml2obj/ELF/retain-section.yaml
47

It doesn't really matter in this case. I'd probably keep the indentation to a minimum - see my inline edit for a suggestion.

47–48
This revision was landed with ongoing or failed builds.Feb 23 2022, 6:42 AM
This revision was automatically updated to reflect the committed changes.
ro added inline comments.Feb 23 2022, 6:46 AM
llvm/test/tools/yaml2obj/ELF/retain-section.yaml
47

Thanks, I went for that: the indentation doesn't convey useful information in this case.