Rework WITHOUT_LLD/TOOLCHAIN fix from r327892 for cross-tools.
ClosedPublic
Actions

Authored by bdrewery on Jun 15 2018, 10:37 PM.

Details

Reviewers

dim
emaste

Commits

rS335448: Rework WITHOUT_LLD/TOOLCHAIN fix from r327892 for cross-tools.

Summary

Also correct an invalid check on MK_LLD_IS_LD for determining which
sources to link into libllvm. If lld is being built at all these
sources are needed. Having a link is irrelevant.

MK_LLD is for the installed lld while MK_LLD_BOOTSTRAP is for the build
tool. For WITH_SYSTEM_LINKER it is necesarry to separate the logic of
these two. When building libllvm TOOLS_PREFIX will be defined and
MK_LLD_BOOTSTRAP should be checked instead.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17462
Build 17292: arc lint + arc unit

Event Timeline

bdrewery created this revision.Jun 15 2018, 10:37 PM

Harbormaster completed remote builds in B17321: Diff 43855.Jun 15 2018, 10:37 PM

bdrewery added a child revision: D15894: Add WITH_SYSTEM_LINKER, on by default, that avoids building lld when possible..Jun 19 2018, 1:18 AM

Would it make sense to set some new variable based on TOOLS_PREFIX, MK_LLD, MK_LLD_BOOTSTRAP so that the same expression doesn't need to be repeated?

bdrewery added inline comments.Jun 19 2018, 1:56 AM

lib/clang/libllvm/Makefile
1278–1279	I'm not sure what's going on here. What is `SRCS_MIW`? Should this really be `MK_LLD` ?

bdrewery added inline comments.Jun 19 2018, 2:02 AM

lib/clang/libllvm/Makefile
1278–1279	Or maybe it needs to be similar to the checks below based on `TOOLS_PREFIX`. Like does it actually matter if `ld` is lld here despite me thinking it didn't?

emaste added inline comments.Jun 19 2018, 2:19 AM

lib/clang/libllvm/Makefile
1278–1279	Introduced in D8879, Use `SRC_MIW` to indicate minimal sources for the "world" stage. Similarly, `SRC_XDW` for world with clang extras and lldb.

emaste added inline comments.Jun 19 2018, 2:24 AM

lib/clang/libllvm/Makefile
1278–1279	`SRC_MIW` was D8879, `MK_LLD_IS_LD` comes from D9226, which is a leftover from earlier when we had only `MK_LLD` and `MK_LLD_IS_LD`. I think your version (!`TOOLS_PREFIX` \|\| `MK_LLD_BOOTSTRAP`) is correct.

bdrewery added inline comments.Jun 19 2018, 3:29 PM

lib/clang/libllvm/Makefile
1278–1279	Well what is so special about SRCS_MIW for world? The point of SYSTEM_LINKER is that /usr/bin/ld should match exactly what is used in WORLDTMP, so it can cross-link all lld-safe archs. So this "special world" var list makes me wonder if it should really always be built for `MK_LLD`, regardless of whether it is the bootstrap linker or not.

bdrewery added inline comments.Jun 19 2018, 3:51 PM

lib/clang/libllvm/Makefile
1278–1279	These used to be in `SRCS_EXT` for `CLANG_EXTRAS` but were enabled once llvm-objdump was enabled by default and at the time not for the cross-linker in WORLDTMP but only for the installed version. rS310775 D8879 +.if !defined(TOOLS_PREFIX) +SRCS_ALL+= ${SRCS_MIW} +.endif So now I'm not sure why `W` was chosen since they are explicitly not added in for the world stage (when `TOOLS_PREFIX` is defined) I'm going to see exactly which of these lld needs and move them to a different list that is more proper (not based on just `LLD_BOOTSTRAP`)

dim added inline comments.Jun 19 2018, 4:06 PM

lib/clang/libllvm/Makefile
1278–1279	Yes, this was split off so llvm-objdump could be built by default. `SRCS_MIW` was just a symbolic choice, I tried to keep to three-letter acronyms, but maybe "world" was not the right one. The idea is that you probably don't need any objdump binary as a cross-tool, but you only need it for the 'final' world build, so it can be installed.

bdrewery added inline comments.Jun 19 2018, 4:10 PM

lib/clang/libllvm/Makefile

1278–1279

Makes sense. I'm betting most of SRCS_MIW is not need for lld. Perhaps only these 3 or only the last. Testing that...

-SRCS_EXT+=     DebugInfo/Symbolize/SymbolizableObjectFile.cpp
-SRCS_EXT+=     DebugInfo/Symbolize/Symbolize.cpp
+SRCS_MIW+=     DebugInfo/Symbolize/SymbolizableObjectFile.cpp
+SRCS_MIW+=     DebugInfo/Symbolize/Symbolize.cpp
...
-SRCS_EXT+=     Object/SymbolSize.cpp
+SRCS_MIW+=     Object/SymbolSize.cpp

bdrewery added inline comments.Jun 19 2018, 11:18 PM