Re-add ALLOW_MIPS_SHARED_TEXTREL, sprinkle it around
ClosedPublic
Actions

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

Details

Reviewers

emaste
dim
arichardson

Commits

rS353140: Re-add ALLOW_MIPS_SHARED_TEXTREL, sprinkle it around

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 - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 26542

Event Timeline

kevans created this revision.Sep 18 2019, 2:11 AM

Herald added subscribers: imp, bdrewery. · View Herald TranscriptSep 18 2019, 2:11 AM

Harbormaster completed remote builds in B26542: Diff 62245.Sep 18 2019, 2:11 AM

dim added inline comments.Sep 18 2019, 6:02 AM

share/mk/bsd.lib.mk
286	Maybe rename this to `ALLOW_MIPS_SHARED_TEXTREL` or something, if it really only applies to mips?
290	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	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	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	Simon plans to fix it, but he was not able to get me definitive timeline -- which is OK, since he does so much. =)

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.

Harbormaster completed remote builds in B26683: Diff 62533.Sep 25 2019, 2:27 AM

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).

In D21696#475458, @arichardson wrote:

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.Sep 27 2019, 1:04 PM

share/mk/bsd.lib.mk
285	why `yes` here and empty string elsewhere?
309–311	Can we extend this comment slightly to indicate the conditions for eventual revert? Presumably a later version of lld would address this?

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.

Harbormaster completed remote builds in B26835: Diff 62847.Oct 2 2019, 5:54 PM