Page MenuHomeFreeBSD

Re-add ALLOW_MIPS_SHARED_TEXTREL, sprinkle it around
ClosedPublic

Authored by kevans on Sep 18 2019, 2:11 AM.

Details

Summary

Diff partially stolen from CheriBSD; these bits need -Wl,-z,notext in order to build in an LLVM world. They are needed for all flavors/sizes of MIPS. This will eventually get fixed in LLVM, but it's unclear when.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kevans created this revision.Sep 18 2019, 2:11 AM
dim added inline comments.Sep 18 2019, 6:02 AM
share/mk/bsd.lib.mk
286 ↗(On Diff #62245)

Maybe rename this to ALLOW_MIPS_SHARED_TEXTREL or something, if it really only applies to mips?

290 ↗(On Diff #62245)

I wonder if there is a way to really "fix" this, in the sense that we could get clang and lld to stop attempting to make those relocations? :)

297 ↗(On Diff #62245)

Looking at this, I think that you should simply check if ALLOW_SHARED_TEXTREL is defined or not, and stop using the "yes" and "no" values, as this is not a regular MK_FOO knob.

Otherwise, you would have to check for:

.if !defined(ALLOW_SHARED_TEXTREL) || ${ALLOW_SHARED_TEXTREL} != "yes"

instead.

301 ↗(On Diff #62245)

Maybe best to decouple this from the -z,notext handling, as people might still want to have linker warnings on mips.

I'll address the rest of this later this morning.

share/mk/bsd.lib.mk
290 ↗(On Diff #62245)

Simon plans to fix it, but he was not able to get me definitive timeline -- which is OK, since he does so much. =)

kevans updated this revision to Diff 62533.Wed, Sep 25, 2:27 AM
kevans marked 3 inline comments as done.
kevans retitled this revision from Re-add ALLOW_SHARED_TEXTREL, sprinkle it around to Re-add ALLOW_MIPS_SHARED_TEXTREL, sprinkle it around.

Rename to ALLOW_MIPS_SHARED_TEXTREL and clean up the bsd.lib.mk segment quite a bit. Stop with silly yes/no, check for defined and only do something if mips.

Looks good. Although I'm not sure if we want this to be conditional on clang/lld since modern GCC+bfd should work (and maybe even clang+new bfd).

Looks good. Although I'm not sure if we want this to be conditional on clang/lld since modern GCC+bfd should work (and maybe even clang+new bfd).

IIRC clang+new bfd still fails as bfd checks object files and catches the relocation in read-only segment. I'll retest that, though, because my memory is dicy here.

emaste added inline comments.Fri, Sep 27, 1:04 PM
share/mk/bsd.lib.mk
285 ↗(On Diff #62533)

why yes here and empty string elsewhere?

297–299 ↗(On Diff #62533)

Can we extend this comment slightly to indicate the conditions for eventual revert? Presumably a later version of lld would address this?

kevans updated this revision to Diff 62847.Wed, Oct 2, 5:53 PM
kevans marked an inline comment as done.
  • One last instance of "yes" where the value doesn't matter has been fixed.
  • Add enough explanation to make it clear that this won't spontaneously go away due to work in the FreeBSD tree, but rather from clang/lld revisions.
emaste accepted this revision.Wed, Oct 2, 6:23 PM

looks ok to me

This revision is now accepted and ready to land.Wed, Oct 2, 6:23 PM
arichardson accepted this revision.Wed, Oct 2, 6:57 PM
This revision was automatically updated to reflect the committed changes.