This is an archive of the discontinued LLVM Phabricator instance.

Initial patch for PS4 toolchain
ClosedPublic

Authored by filcab on Jul 16 2015, 2:59 PM.

Details

Summary

This patch adds missing pieces to clang, including the PS4 toolchain
definition, added warnings, PS4 defaults, and Driver changes needed for
our compiler.

Patch extracted by me, Katya Romanova, and Pierre Gousseau.

Diff Detail

Event Timeline

filcab updated this revision to Diff 29949.Jul 16 2015, 2:59 PM
filcab retitled this revision from to Initial patch for PS4 toolchain.
filcab updated this object.
filcab added reviewers: kromanova, alexr, echristo.
filcab added a subscriber: cfe-commits.
kromanova edited edge metadata.Jul 17 2015, 11:53 AM

LGTM. However, since we were preparing a patch together with Filipe and Pierre, I'm as bias as they are :) We addressed all my comments before Filipe submitted this patch.
It will be nice if someone else (and preferably someone outside of Sony) reviews the patch.
@Eric, could you please review?

Kattya.

asl added a subscriber: asl.Jul 17 2015, 1:36 PM
filcab updated this revision to Diff 30062.Jul 17 2015, 6:32 PM
filcab edited edge metadata.

Update for clang API change.
Remove a PS4-specific env variable from the 'dangerous' list.

echristo edited edge metadata.Jul 29 2015, 3:40 PM

Added a bunch of inline comments. The easiest way for this is probably going to be to split this out into a few separate patches where we get the skeleton in and then start filling it out.

Thanks!

-eric

include/clang/Basic/DiagnosticDriverKinds.td
203–208

Are the existing header warnings insufficient?

include/clang/Driver/Options.td
1159–1160

Can this be done separately?

lib/Driver/Tools.cpp
3110–3116

If you could rewrite and separate this out a bit more it'd be great.

3585–3586

Hmm?

3608

Ditto.

9433–9434

Using an external assembler rather than the integrated one?

9731–9734

Based on this (and the above code) might we want an enum set of options to the linker bit? How about we split up this patch and leave the linker option separate and we can talk about and add that bit after?

lib/Driver/Tools.h
783

Hrm. PS4 maybe? PS4cpu seems to say that it's a custom cpu target and not a custom environment?

lib/Frontend/InitHeaderSearch.cpp
325–344

This all seems odd, what's going on here?

alexr accepted this revision.Jul 29 2015, 6:03 PM
alexr edited edge metadata.

LGTM with the comment rewrite that Eric asked for.

This revision is now accepted and ready to land.Jul 29 2015, 6:03 PM
filcab planned changes to this revision.Jul 30 2015, 10:55 PM

Thank you, Eric.
We'll address the comments and post back an updated patch. Probably make the linker a separate patch and submit it before we update this one.

I would prefer to not split this into "add the toolchain, then fill it out more" (if possible), since we already have this in our internal branch. Having two bigger merges (especially if one is "non-working") or waiting until we have everything to do the merge wouldn't be ideal.

lib/Driver/Tools.cpp
3110–3116

I'm not the best at it, but I'll try :-)

3585–3586

We have different defaults from other platforms.

3608

Ditto, different defaults.
But I guess I can hoist out the Triple.isPS4CPU() on both cases, if you prefer, like it's done for other toolchains like:

bool IsWindowsMSVC = getToolChain().getTriple().isWindowsMSVCEnvironment();
9433–9434

We support both (but default to integrated, since we don't override Generic_GCC::IsIntegratedAssemblerDefault()).

lib/Driver/Tools.h
783

PS4cpu (or PS4CPU) is more consistent with how we've been naming things in open source.

filcab marked an inline comment as done.Aug 3 2015, 3:34 PM
filcab added inline comments.
include/clang/Basic/DiagnosticDriverKinds.td
203–208

Yes. We have an expected location and would like the whole "PS4 SDK headers" thing in there.
But I can change it to be more general, like:

def warn_drv_unable_find_directory_expected : Warning<
  "unable to find %0 directory, expected to be in '%1'">,
  InGroup<InvalidOrNonExistentDirectory>;

That might be preferable. That way other toolchains can use it (and we can merge the "libraries" and "headers" warnings.

include/clang/Driver/Options.td
1159–1160
kromanova added inline comments.Sep 2 2015, 4:12 PM
lib/Frontend/InitHeaderSearch.cpp
325–344

Our SDK layout structure is different from other platforms. Some of the PS4 specific headers (e.g. graphic headers, etc) are placed into target/include_common directory and we want to have them in the search path.

Replying to the inline comments. I know we're waiting on the linker stuff, but Katya made me realize I hadn't replied to your comments.

-eric

include/clang/Basic/DiagnosticDriverKinds.td
203–208

Seems totally reasonable :)

I can see it being something that you'd want to enable to make sure that sysroot prefixes are paid attention to and, e.g. /usr/include is never touched.

lib/Driver/Tools.cpp
3585–3586

*nod* disabling column info yes? I have "opinions" on this, but it's not my platform. Needs a comment.

3608

Probably better to just handle it the same way as the column info I'd think. Also, this is all terrible and needs to change, that said, not your problem. :)

lib/Driver/Tools.h
783

Sure.

lib/Frontend/InitHeaderSearch.cpp
325–344

You also check for ps4cpu in here?

kromanova added inline comments.Sep 2 2015, 4:39 PM
lib/Frontend/InitHeaderSearch.cpp
325–344

I don't think this is needed since we are under Triple::PS4 switch now. Sorry, this is an oversight on our part, caused by refactoring of old code that didn't has PS4 switch case before. We will remove 'if' part.

jroelofs added inline comments.
lib/Frontend/InitHeaderSearch.cpp
340

The lifetime of P ends here, yet a reference to it lives on because of the = P.str(). Using it later on line 344 is UB.

kromanova added inline comments.Sep 2 2015, 5:03 PM
lib/Frontend/InitHeaderSearch.cpp
340

Good catch. Thank you! We will fix it.

kromanova added inline comments.Sep 3 2015, 12:11 AM
lib/Driver/Tools.cpp
3585–3586

Our debugger doesn't use column information. Though it's not too much, unused column information adds to the total size of DI.
We will add a comment.

3608

We could do exactly the same for aranges as it we did for the column info. Though it seems that we will have to add 'gno_dwarf_aranges' if we want to use hasFlag function here too. Is it worth doing it?

jroelofs added inline comments.Sep 3 2015, 8:39 AM
lib/Frontend/InitHeaderSearch.cpp
340

Oops, nevermind... I was wrong: BaseSDKPath is a std::string, not a char* so there's no lifetime issue here.

filcab updated this revision to Diff 34522.Sep 10 2015, 6:47 PM
filcab marked 12 inline comments as done.
filcab edited edge metadata.

Addressed review comments.

This revision is now accepted and ready to land.Sep 10 2015, 6:47 PM
echristo accepted this revision.Sep 14 2015, 1:07 PM
echristo edited edge metadata.

One set of inline comments, and also:

test/Driver/Inputs/scei-ps4_tree/target/include_common/.keep

eh?

-eric

lib/Driver/Tools.cpp
9699–9703

How does this work with the things below that check for linker name?

Also seems like we'd really want to be able to check our host using the normal ways rather than an ifdef. This can be done as a followup, but do please check into this.

test/Driver/Inputs/scei-ps4_tree/target/include_common/ and test/Driver/Inputs/scei-ps4_tree/target/include/ are expected to be present by a test ps4-header-search.c. This test checks that these directories are found in the header search path. If these directories are not found, the compiler complains and the test fails.
.keep file is needed to keep these directories in place.

lib/Driver/Tools.cpp
9699–9703

We will check for the host without the ifdef, but we will do it in a follow-up commit.

test/Driver/Inputs/scei-ps4_tree/target/include_common/ and test/Driver/Inputs/scei-ps4_tree/target/include/ are expected to be present by a test ps4-header-search.c. This test checks that these directories are found in the header search path. If these directories are not found, the compiler complains and the test fails.
.keep file is needed to keep these directories in place.

How about just a README file in the directories saying this rather than the .keep file? :)

-eric

lib/Driver/Tools.cpp
9699–9703

That's fine, thanks.

kromanova added inline comments.Sep 16 2015, 3:17 PM
lib/Driver/Tools.cpp
9699–9703

I think that there is a problem with this code and setting up LinkerOptName variable below (at least things are inconsistent). However, it only creates a problem for Linux hos for P4 compiler t (which we don't care about much). If you don't mind, we will fix it in a separate follow-up commit (along with ifdef).

Hi Eric,
I don't mind changing .keep into README. However, originally we did it for consistency purpose. If you grep for .keep in /llvm/tools/clang/test/Driver/Inputs directory, you will find hundreds of instances of these files that were created for similar reasons (checking SDK layout), but you won't find a single README file there. Do you still want us to make this change?

Hi Eric,
I don't mind changing .keep into README. However, originally we did it for consistency purpose. If you grep for .keep in /llvm/tools/clang/test/Driver/Inputs directory, you will find hundreds of instances of these files that were created for similar reasons (checking SDK layout), but you won't find a single README file there. Do you still want us to make this change?

Huh. I guess not.

(Well, I mean yes, but if I want that done I can do it myself and keep consistency in this commit).

-eric

silvas added a subscriber: silvas.Sep 18 2015, 4:26 PM

Once this lands, I think we can revert r247977

This revision was automatically updated to reflect the committed changes.

Would this cause bunch of failures if target is *-ps4 and SDK_DIR is empty?

clang-3.8: error: unable to find PS4 system headers directory, expected to be in '/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/../../target/include'