[PATCH] D131225: Driver: Refactor and support per target dirs in baremetal

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added inline comments.


================
Comment at: clang/include/clang/Driver/ToolChain.h:388
+  /// IsRISCVBareMetal - Does this tool chain is a riscv baremetal target.
+  static bool IsRISCVBareMetal(const llvm::Triple &);
+
----------------
barannikov88 wrote:
> barannikov88 wrote:
> > manojgupta wrote:
> > > barannikov88 wrote:
> > > > 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.
> > > > What was wrong with the previous implementation that made you move the methods here?
> > > 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.
> > > I think moving them to Triple class (Triple.h) is a clearer option.
> > Is the triple is all that is necessary to decide whether the target is bare metal or not?
> > Sounds interesting, but one may argue that Triple should not know about toolchains (like it should not know about C data type bit widths, for example).
> > What if just add a few switch-cases to Driver::getToolChain as for every other toolchain? Retaining the former static method 'BareMetalToolChain::handlesTarget' is still better in my opinion.
> > They can't be made virtual since the checks need to happen before instantiating ToolChain class. 
> 
> 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.
> To //instantiate// BareMetalToolchain you need another function (like the former BareMetalToolChain::handlesTarget, or the suggested approach with Triple). For these instantiations `IsBareMetal` will return true, and false for all other instantiations which are not bare metal.
> 
>Is the triple is all that is necessary to decide whether the target is bare metal or not?

At least for baremetal, from what I see, triple seems to be the way how baremetal toolchain is decided in practice.

Examples: 
1. https://gitweb.gentoo.org/proj/crossdev.git/tree/crossdev#n331 (		# Bare metal targets 		*-newlib|*-elf|*-eabi|*-rtems*)
2. https://elinux.org/images/1/15/Anatomy_of_Cross-Compilation_Toolchains.pdf ( arm-foo-none-eabi, bare-metal toolchain targeting the ARM architecture, from
vendor foo )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131225/new/

https://reviews.llvm.org/D131225