This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Make --discard-locals default on riscv

Authored by abrachet on Mar 8 2023, 6:10 PM.



This matches bfd.ld's behavior

Diff Detail

Event Timeline

abrachet created this revision.Mar 8 2023, 6:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 6:10 PM
abrachet requested review of this revision.Mar 8 2023, 6:10 PM
jrtc27 added inline comments.Mar 8 2023, 6:13 PM
717 ↗(On Diff #503587)

Seems like the semantics of Default should change, otherwise it's not the default?..

jrtc27 added a comment.Mar 8 2023, 6:14 PM

Anyway this seems to contradict the approach chosen by D127826

mcgrathr added inline comments.
717 ↗(On Diff #503587)

Agreed. The only meaning of Default really is what the final case in shouldKeepInSymtab does, and I think it makes more sense to just change that one if to add an || config->emachine == EM_RISCV there.

abrachet updated this revision to Diff 503590.Mar 8 2023, 6:20 PM
abrachet marked 2 inline comments as done.
jrtc27 added inline comments.Mar 8 2023, 6:20 PM

Comment is now wrong...

abrachet updated this revision to Diff 503591.Mar 8 2023, 6:22 PM
abrachet marked an inline comment as done.
mcgrathr accepted this revision.Mar 8 2023, 6:24 PM
mcgrathr added inline comments.

Indeed. This merits a nontrivial comment anyway, something like " * RISC-V, where the assembler doesn't discard them by default as other targets do, so as to facilitate relaxation that can move things around and would break if labels were reduced to section offsets".

This revision is now accepted and ready to land.Mar 8 2023, 6:24 PM
jrtc27 added inline comments.Mar 8 2023, 6:24 PM
  • A or B
  • C

when it's already implicitly or'ed is weird, be consistent and do

  • A
  • B
  • C
jrtc27 added a comment.Mar 8 2023, 6:25 PM

Anyway, this definitely still merits a review from @MaskRay given his commit to tackle the problem differently

MaskRay requested changes to this revision.EditedMar 9 2023, 12:34 AM

Yeah; I wish that we don't do this. The target difference doesn't seem useful.

IIRC even a RISC-V binutils maintainers said the behavior is strange but perhaps people don't have enough motivation to fix that.
We don't really need to copy the behavior as Clang Driver has passed -X.

This revision now requires changes to proceed.Mar 9 2023, 12:34 AM
abrachet abandoned this revision.Mar 9 2023, 12:02 PM