This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Include libclang_rt.builtins in the standard way
ClosedPublic

Authored by sbc100 on Oct 23 2017, 5:14 PM.

Event Timeline

sbc100 created this revision.Oct 23 2017, 5:14 PM
sbc100 updated this revision to Diff 119977.Oct 23 2017, 5:24 PM
  • fix tests
sbc100 updated this revision to Diff 119978.Oct 23 2017, 5:25 PM
  • remove debugging
sbc100 added a subscriber: cfe-commits.
dschuff added inline comments.Oct 23 2017, 5:41 PM
lib/Driver/ToolChain.cpp
318

Is this logic intended to replace what was removed from CommonArgs.cpp? Should there be an assert here too?

sbc100 added inline comments.Oct 24 2017, 10:12 AM
lib/Driver/ToolChain.cpp
318

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.

dschuff accepted this revision.Oct 24 2017, 10:25 AM
dschuff added inline comments.
lib/Driver/ToolChain.cpp
318

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.

This revision is now accepted and ready to land.Oct 24 2017, 10:25 AM
sbc100 updated this revision to Diff 120111.Oct 24 2017, 12:04 PM
  • remove debugging
  • git squash commit for startup_libs.
  • update test expectations
This revision was automatically updated to reflect the committed changes.