This is an archive of the discontinued LLVM Phabricator instance.

[V2] MC: make section classification a bit more thorough
ClosedPublic

Authored by compnerd on Sep 7 2022, 2:52 PM.

Details

Summary

This does *NOT* change the emitted section flags in any way. This only
impacts the internal classification of sections.

Extend the section classification in LLVM for ELF targets. This has one
important change: we now classify sections as text by default rather
than readonly. This matches the behaviour for GAS better.

Ensure that any section that has a writable attribute set is not treated
as readonly. We also special case any section named .debug_ which is
reserved for DWARF as metadata. In the case none of the attributes are
set (or because no attributes were provided), consult the section name
for classification. We match the well known names and classify the
section accordingly. Any remaining section is now classified as text.

This change allows us to classify sections in the MC layer more
precisely which is needed for subsequent changes for handling target
specific behaviour.

Re-apply the change that was reverted with additional changes to
classify section prefixes appropriately and differentiate the TLS
sections, addressing the FIXME and post-commit review comments by
@MaskRay.

Diff Detail

Event Timeline

compnerd created this revision.Sep 7 2022, 2:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 2:52 PM
compnerd requested review of this revision.Sep 7 2022, 2:52 PM
compnerd updated this revision to Diff 458833.Sep 8 2022, 12:49 PM
MaskRay added inline comments.Sep 8 2022, 9:51 PM
llvm/lib/MC/MCContext.cpp
567

I think !(... & ...) is more common

576

Drop all gnu.linkonce and llvm.linkonce. linkonce is used before COMDAT era (<2000) and llvm never generates it.

580

.data.rel.ro.* is possible.

583

Instead of a StringSwitch, consider

if (Name.consumes_front(".tbss") && (Name.empty() || Name[0] == '.'))
  ...

so that the many prefixes don't have to be repeated for Case/StartsWith.

592

Is default SectionKind::getText() instead of SectionKind::getReadOnly() desired? I expect that text is covered by a previous if: Flags & ELF::SHF_EXECINSTR

This has one important change: we now classify sections as text by default rather than readonly. This matches the behaviour for GAS better.

This is somewhat surprising to me. The default section (before a .section directive) in gas is .text but that does not indicate other sections?

compnerd added inline comments.Sep 9 2022, 6:54 AM
llvm/lib/MC/MCContext.cpp
592

This is the core of the change itself - and what is needed for subsequent changes.

compnerd updated this revision to Diff 459488.Sep 12 2022, 9:28 AM
compnerd marked 3 inline comments as done.
MaskRay added inline comments.Sep 21 2022, 10:38 AM
llvm/lib/MC/MCContext.cpp
587

As discussed, we need a comment that we choose SectionKind::getText() because of an unknown Kind (like a forward declaration) and this is used by R_RISCV_{ADD,SUB}*

compnerd updated this revision to Diff 461959.Sep 21 2022, 11:23 AM
compnerd marked an inline comment as done.
compnerd added inline comments.
llvm/lib/MC/MCContext.cpp
587

Added such a comment. Happy to adjust the comment if you feel it's unclear or overly verbose.

MaskRay accepted this revision.Sep 21 2022, 12:40 PM
This revision is now accepted and ready to land.Sep 21 2022, 12:40 PM
This revision was landed with ongoing or failed builds.Sep 22 2022, 9:27 AM
This revision was automatically updated to reflect the committed changes.
compnerd marked an inline comment as done.