This is an archive of the discontinued LLVM Phabricator instance.

Add driver support for Native Client SDK
ClosedPublic

Authored by dschuff on Mar 24 2015, 2:56 PM.

Details

Summary

Add Tool and ToolChain support for clang to target the NaCl OS using the NaCl
SDK for x86-32, x86-64 and ARM.

Includes nacltools::Assemble and Link which are derived from gnutools. They
are similar to Linux but different enought that they warrant their own class.
Also includes a NaCl_TC in ToolChains derived from Generic_ELF with library
and include paths suitable for an SDK and independent of the system tools.

Diff Detail

Event Timeline

dschuff updated this revision to Diff 22603.Mar 24 2015, 2:56 PM
dschuff retitled this revision from to Add driver support for Native Client SDK.
dschuff updated this object.
dschuff added a reviewer: jvoung.
dschuff added a subscriber: Unknown Object (MLST).
jvoung added inline comments.Mar 25 2015, 2:41 PM
lib/Basic/Targets.cpp
3901

BTW, for the description string there was some test that did:

// RUN: %clang_cc1 -triple arm-nacl-gnueabi

wonder if it should just change to arm-nacl or arm-nacl-gnueabihf.

test/CodeGen/target-data.c

lib/Driver/ToolChains.cpp
2399

parameters don't seem lined up here and "AddClangCXXStdlibIncludeArgs"

2400

The "warning" part is a bit confusing on first read. You have to look at the implementation of "GetCXXStdlibType" to really know that there's warning.

2447

Is there anywhere that would warn if plain "gnueabi" was specified?

lib/Driver/Tools.cpp
7920

line up params

7939

Add a period at the end of the sentence.

7950

Technically there is OPT_pie too, comparing this to the gnutools version.

"""
OPT_dynamic looks like a darwin thing. Under the GCC man page:
-dynamic
-...

These options are passed to the Darwin linker. The Darwin linker man page describes them in detail.
"""

7984

Should explicitly check for x86-64 and have an llvm_unreachable or error for unexpected arches.

8013

Does a ref work?

const ToolChain::path_list &Paths = ...

8041

Might not need to mention PPAPI or phrase this differently, since it's not necessarily nacl-clang is not browser-specific.

dschuff updated this revision to Diff 22758.Mar 26 2015, 2:49 PM
  • jvoung review
dschuff updated this revision to Diff 22759.Mar 26 2015, 2:51 PM
  • missed duped comment
lib/Basic/Targets.cpp
3901

Just made it arm-nacl (the environment shouldn't affect the datalayout anyway)

lib/Driver/ToolChains.cpp
2399

Fixed

2400

made it clearer.

2447

This just covers the case if it's not specified. If gnueabi is specified that should override this default and be honored. Since the behavior of the regular arm compiler is just to allow the user to do that, maybe we should too?

lib/Driver/Tools.cpp
7920

done.

7939

done.

7950

Yeah... because we default to static, we wanted a way to sort of support doing dynamic, at least until we can make it the default. we don't support PIE at all yet.

7984

done. made it a diag since putting an unexpected arch on the command line actually does make it all the way to link here.

8013

yes done. im guessing that was probably the original intention since it's already const.

8041

done.

jvoung accepted this revision.Mar 27 2015, 1:23 PM
jvoung edited edge metadata.

otherwise LGTM

a little sad that a bunch of tools have to duplicate the "// Silence warning for "clang -g foo.o -o foo"" stuff... =/

lib/Driver/ToolChains.cpp
2447

If gnueabi is allowed, then perhaps the "StringRef tools::arm::getARMFloatABI()" code should check for it, rather than force hard-float.

lib/Driver/Tools.cpp
675

See reply about allowing gnueabi in ComputeEffectiveClangTriple... how does that interact with this then? Should this check for GNUEABI and set FloatABI = "soft"?

7941

params can be lined up

lib/Driver/Tools.h
530

Line up thy parameters.

This revision is now accepted and ready to land.Mar 27 2015, 1:23 PM
dschuff updated this revision to Diff 22904.Mar 30 2015, 1:32 PM
dschuff edited edge metadata.
  • jvoung review
  • missed duped comment
  • remove special snowflakes for AddARMTargetArgs and getARMFloatABI
lib/Driver/ToolChains.cpp
2447

OK, done. this means we can just fall into the default case and be more like Linux, both there and clang::AddARMTargetArgs() which is probably a good thing. I did those changes before adding this code to set the default in a later CL... it ended up being necessary for other reasons (https://codereview.chromium.org/838933004). This seemed like a bit more dubious change at the time but it's nice in that it's localized to NaCl_TC and that it allows fewer exceptional cases elsewhere.

I think this is fine and can land for now, but more generally, do agree that it's nice to allow environments other than gnueabihf and/or to do so without warning?

This revision was automatically updated to reflect the committed changes.