Refactor baremetal driver code to reduce the bespoke
additions and base class overrides.
This lets us use the per target runtimes like other clang
targets. E.g. clang -target armv7m-cros-none-eabi will now
be able to use the runtimes installed at
<resource_dir>/lib/armv7m-cros-none-eabi instead of the hardcoded
path <resource_dir>/lib/baremetal.
The older code paths should still continue to work as before if
<resource_dir>/lib/<tuple> does not exist.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I think this patch is moving the things in the right direction. But please wait for some other reviewers to chime in.
It seems quite opposite to me.
clang/include/clang/Driver/ToolChain.h | ||
---|---|---|
384 ↗ | (On Diff #450198) | Is this a correct sentence? (My English is poor.) |
388 ↗ | (On Diff #450198) | The ToolChain class is an interface class. It is strange to see such kind of methods here. IsBareMetal should at least be virtual and overridden in concrete implementation of baremetal toolchains. IsRISCVBareMetal should not be here at all. |
To be clear, my comment was aimed at using target specific directory instead of using lib/baremetal.
clang/include/clang/Driver/ToolChain.h | ||
---|---|---|
384 ↗ | (On Diff #450198) | I'll fix it. Somehow I forgot to check them. |
388 ↗ | (On Diff #450198) | There is a need to check for IsBareMetal and variants. They can't be made virtual since the checks need to happen before instantiating ToolChain class. I think moving them to Triple class (Triple.h) is a clearer option. |
clang/include/clang/Driver/ToolChain.h | ||
---|---|---|
388 ↗ | (On Diff #450198) |
Is the triple is all that is necessary to decide whether the target is bare metal or not? |
clang/include/clang/Driver/ToolChain.h | ||
---|---|---|
388 ↗ | (On Diff #450198) |
This is kind of two different things. It can be made virtual. The name of the method IsBareMetal of the ToolChain class suggests that it is checking whether this concrete instance of the ToolChain is baremetal. This (virtual) method could be used in getCompilerRTPath, for example. |
clang/include/clang/Driver/ToolChain.h | ||
---|---|---|
388 ↗ | (On Diff #450198) |
At least for baremetal, from what I see, triple seems to be the way how baremetal toolchain is decided in practice. Examples:
vendor foo ) |
llvm/include/llvm/ADT/Triple.h | ||
---|---|---|
916 | Any future change to these methods will force recompilation of the whole project. It is better to move the definitions into the corresponding cpp file. |
LGTM with the style issues fixed, but I'd like someone else to take a look.
clang/lib/Driver/ToolChains/BareMetal.cpp | ||
---|---|---|
228–236 | There are a few style issues.
Same applies to ConstructJob below, there are also redundant braces in for statement. |
llvm/lib/Support/Triple.cpp | ||
---|---|---|
1936 | (nit) at least llvm:: qualification is redundant, Triple:: might also be. |
clang/lib/Driver/ToolChains/BareMetal.cpp | ||
---|---|---|
228–236 | Addressed most. warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl] |
Going back to older handlesTarget() style in Baremetal.
Apparently RISCV can be both Baremetal or RISCVToolchain for the
same tuple. I do not know of the nuances so trying not to disturb that for
now.
Line overflowed