[PATCH] D39218: [WebAssembly] Include libclang_rt.builtins in the standard way

[Derek Schuff via Phabricator via cfe-commits]

dschuff accepted this revision.
dschuff added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/Driver/ToolChain.cpp:318
+  else
+    OSLibName = getOS();
   llvm::sys::path::append(Path, "lib", OSLibName);
----------------
sbc100 wrote:
> dschuff wrote:
> > Is this logic intended to replace what was removed from CommonArgs.cpp? Should there be an assert here too?
> Just didn't see the point of that assert.  Can you see what the intention might be?  I don't see why AddRunTimeLibs() should be callable for any/all triples, do you?  I would have had to add  llvm::Triple::Unknown to the list of supported OSs, which seemed strange.
I assume it was just because different OSes have different conventions for the rtlib, and if the OS is unknown and/or there's no particular support, then there might be a bug. But OTOH it doesn't seem bad to have some reasonable default either. For that matter it also seems a little weird that now we are letting the binary format be the determining thing for the Compiler-RT path. But I guess the real issue is that none of the OS, arch, or binary format is really the determining thing; it's really the toolchain/SDK or distribution of the compiler that determines what the path should be, and that can be affected by a lot of other things (e.g. is it the "system" compiler or not, is it a cross compiler, etc). So... yeah I think having this as a default makes as much sense as asserting, and when someone needs to add support for their distribution, then I suppose it's on them to refactor as needed and keep the tests working.


https://reviews.llvm.org/D39218