Page MenuHomeFreeBSD

Don't hardcode X_ prefix on fallback path for X_COMPILER_*/X_LINKER_*
AbandonedPublic

Authored by jrtc27 on Nov 2 2018, 6:07 PM.

Details

Summary

On the fallback path, we hardcode the X_ prefix to the info variables.
Since the only prefixed tool in _cc_vars/_ld_vars is XCC/XLD this works
for us at the moment, but potentially breaks for anyone adding another
tool type to _cc_vars/_ld_vars, clobbering the wrong variables and
leaving the correct variables unset. Instead we should be more general
and use the X_ variable's contents as the prefix.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 20599
Build 20020: arc lint + arc unit

Event Timeline

jrtc27 created this revision.Nov 2 2018, 6:07 PM
jrtc27 updated this revision to Diff 49952.Nov 2 2018, 6:19 PM

Do the same for X_LINKER_* too

jrtc27 retitled this revision from bsd.compiler.mk: Don't hardcode X_ prefix on fallback path to Don't hardcode X_ prefix on fallback path for X_COMPILER_*/X_LINKER_*.Nov 2 2018, 6:20 PM
jrtc27 edited the summary of this revision. (Show Details)

Looks good to me, but I'll give Bryan a change to opine.

bdrewery requested changes to this revision.Dec 4 2018, 10:36 PM
bdrewery added inline comments.
share/mk/bsd.compiler.mk
212–216

This makes no sense now. COMPILER_TYPE=${COMPILER_TYPE}?

This revision now requires changes to proceed.Dec 4 2018, 10:36 PM
bdrewery added inline comments.Dec 4 2018, 10:42 PM
share/mk/bsd.compiler.mk
212–216

If it wasn't clear, that is the effect of your change. It achieves nothing that I can see.

jrtc27 marked an inline comment as done.Dec 4 2018, 10:43 PM
jrtc27 added inline comments.
share/mk/bsd.compiler.mk
212–216

We only reach this branch if ${cc} != "CC", ie ${X_} != "".

jrtc27 marked an inline comment as done.Dec 4 2018, 10:46 PM
jrtc27 added inline comments.
share/mk/bsd.compiler.mk
212–216

It is an NFC as far as FreeBSD upstream is concerned. However, downstreams exist that add additional variables to _cc_vars, and in that case ${X_} is neither empty nor "X_". Thus, whilst it's "pointless" upstream, it takes no effort to keep upstream and saves having to maintain it as a downstream patch and resolve conflicts every time someone adds a new COMPILER/LINKER_FOO.

jrtc27 marked an inline comment as done.Dec 4 2018, 10:46 PM
jrtc27 added inline comments.
share/mk/bsd.compiler.mk
212–216

This is also what the patch summary is trying to convey, in case it isn't clear.

bdrewery added inline comments.Dec 4 2018, 10:53 PM
share/mk/bsd.compiler.mk
212–216

It's nonsense though and could easily be "fixed" by anyone since it's a bug/pointless to set a variable to itself. Especially without any comments or real reason. Just looking at this file I want to rewrite it since it's such chaos. Your change is easily lost if someone is motivated to do that.

_cc_vars is not supposed to be modifiable. An underscore means that. I don't think it's good to commit hacks for private variables. Can you elaborate on what you have done exactly downstream?

bdrewery added inline comments.Dec 4 2018, 10:57 PM
share/mk/bsd.compiler.mk
212–216

Also I'm not convinced that rS339636 is right which added these variables. If XLD/XCC is *not* set on the command line then that change may be wrong in recursive make.

jrtc27 marked an inline comment as done.Dec 4 2018, 10:59 PM
jrtc27 added inline comments.
share/mk/bsd.compiler.mk
212–216

[..] since it's a bug/pointless to set a variable to itself [..]

We're not setting a variable to itself; this case isn't hit when ${X_} is empty, so we will never be trying to execute COMPILER_TYPE=COMPILER_TYPE.

Can you elaborate on what you have done exactly downstream?

This is in CheriBSD; we've had _cc_vars+= CHERI_CC CHERI_ for years in our bsd.compiler.mk.

jrtc27 marked an inline comment as done.Dec 4 2018, 11:02 PM
jrtc27 added inline comments.
share/mk/bsd.compiler.mk
212–216

Ok, turns out I'm wrong, _cc_vars+= CHERI_CC CHERI_ was added in September, in the precursor to the revision you mentioned.

bdrewery added inline comments.Dec 4 2018, 11:04 PM
share/mk/bsd.compiler.mk
212–216

Looking deeper I think rS339636 is fine but it does leave a desire to refactor significantly.

bdrewery added inline comments.Dec 4 2018, 11:09 PM
share/mk/bsd.compiler.mk
212–216

You want CHERI_COMPILER_TYPE to default to COMPILER_TYPE?

CheriBSD is compiled using 3 different compilers and 3 different linkers?
Exactly where / which phase are these 3rd compiler/linkers used?

jrtc27 marked an inline comment as done.Dec 4 2018, 11:17 PM
jrtc27 added inline comments.
share/mk/bsd.compiler.mk
212–216

Well, yes, that is a current problem and should be fixed by just changing the if to ${cc} == "CC" || ${${cc}} != ${CC} (which is what the intent of the if *really* is). However, theoretically ${CHERI_CC} can be equal to ${CC} (could just use our built LLVM toolchain for both), which would then hit the else and still trash X_COMPILER_TYPE even if XCC is something different.

As for why these exist, it's so pure MIPS stuff can still be built with GCC/binutils. It's quite possible we don't exercise that ability any more, but the flexibility in our build system is there and potentially useful.

arichardson added inline comments.
share/mk/bsd.compiler.mk
212–216

We use /use/bin/cc and /use/bin/LD for bootstrap/build tools. Until fairly recently we were using the bootstrapped GCC 4.2 + bfd as XCC/XLD and our forks of llvm/clang and lld for any code using CHERI capabilities (CHERI_CC/CHERI_LD)
We are now using cheri clang for both MIPS and CHERI code but it is still possible to use the bootstrapped GCC instead.

Some files are compiled with CHERI_CC instead of XCC during the libraries phase since we need to build e.g. a memcpy that works with CHERI capabilities.

We then also have a buildcheri phase similar to lib32 that builds all libraries as using CHERI_CC.

(It's actually slightly more complicated but hopefully this explains where we use the different compilers/linkers)

jrtc27 abandoned this revision.Oct 13 2019, 9:48 PM

We're ditching our extra CHERI_CC/CHERI_LD so this is no longer relevant to us.