compnerd added inline comments.

================
Comment at: cmake/caches/BaremetalARM.cmake:1
+set(LLVM_TARGETS_TO_BUILD ARM CACHE STRING "")
+
----------------
jroelofs wrote:
> compnerd wrote:
> > Please rename this file to `BareMetalARMv6.cmake`.  (I'm interested in the 
> > suffix primarily).
> My plan is to eventually add multilibs to this configuration, so while that 
> makes sense short term, I don't think it makes sense long term.
> 
> With that in mind, do you still want me to rename it?
I think it makes sense longer term still.  ARMv6 vs ARMv7.  Even if you do 
multilib, that wouldnt take care of that right?


================
Comment at: lib/Driver/ToolChains/BareMetal.cpp:68
+  SmallString<128> Dir(getDriver().ResourceDir);
+  llvm::sys::path::append(Dir, "lib", "baremetal");
+  return Dir.str();
----------------
jroelofs wrote:
> compnerd wrote:
> > Why not just the standard `arm` directory?
> There are a few differences between the stuff in the existing ones, and what 
> is needed on baremetal. For example __enable_execute_stack, emutls, as well 
> as anything else that assumes existence of pthreads support shouldn't be 
> there.
Well, I think that "baremetal" here is a bad idea.  How about using the android 
approach?  Use `clang_rt.builtins-arm-baremetal.a` ?


================
Comment at: lib/Driver/ToolChains/BareMetal.cpp:107-108
+                                    ArgStringList &CmdArgs) const {
+  CmdArgs.push_back("-lc++");
+  CmdArgs.push_back("-lc++abi");
+  CmdArgs.push_back("-lunwind");
----------------
jroelofs wrote:
> jroelofs wrote:
> > compnerd wrote:
> > > I think that this is a bit extreme.  We already have `-stdlib=libc++` and 
> > > `-stdlib=libstdc++`.  Why not just honor that?
> > I wasn't going for "support every possible thing out of the tin", instead 
> > preferring incremental development :)
> Added support for `-stdlib=`.
> 
> I made the assumption that `-stdlib=libstdc++` implies the user wants 
> libsupc++ as their ABI library, though someone may want libstdc++ + 
> libc++abi, or other combinations. If that's the case, we can add `-abilib=`, 
> and likewise `-unwinder=`... but let's save that for another patch, on 
> another day.
Yeah, switching between libc++/libc++abi and libstdc++/libsupc++ sounds 
reasonable.  Anything beyond that is something which doesnt work well within 
the driver anyways.  I should polish off my `-unwinder` patch.


================
Comment at: lib/Driver/ToolChains/BareMetal.h:42
+
+  const char *getDefaultLinker() const override { return "ld.lld"; }
+
----------------
jroelofs wrote:
> compnerd wrote:
> > I think that this really should be `ld` still, as that is the canonical 
> > name for the linker.
> You mean `lld`?
> 
> ```
> $ lld
> lld is a generic driver.
> Invoke ld.lld (Unix), ld (macOS) or lld-link (Windows) instead.
> ```
> 
> Or are you saying: "make binutils ld the default, not llvm's lld"?
Im saying use the name "ld".  ld is a symlink on most linux distributions these 
days.  ld -> ld.gold, ld.bfd, ld.lld


https://reviews.llvm.org/D33259



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to