This is an archive of the discontinued LLVM Phabricator instance.

Revised Initial patch for PS4 toolchain
ClosedPublic

Authored by kromanova on Oct 6 2015, 1:26 PM.

Details

Summary

Initial patch for PS4 toolchain was created here: http://reviews.llvm.org/D11279
When this patch was committed in r248546, it caused 2 distinct problems.

(1) Many failures were reported on recently set up PS4 bot. The PS4 driver intentionally reported warnings if PS4 SDK was missing. The new buildbot didn't have PS4 SDK directory installed. This "combination" caused failures for all the tests that had -Werror option.
(2) ps4-linker-win.c failed on all the Windows bots.

This commit was reverted in r248548 to make the bots green again and to decide on how to deal with missing PS4 SDK directory issue.

The new patch should take care of both problems mentioned above.
Since review for http://reviews.llvm.org/D11279 was owned by Filipe Cabecinhas and already closed, it's probably easier for me to open a new code review. Feel free to compare this new patch to the latest patch in D11279.
Here is the summary of the changes:

  1. include/clang/Basic/DiagnosticDriverKinds.td

Added DefaultIgnore attribute to InvalidOrNonExistentDirectory.
By default, the PS4 driver won't report a warning about missing PS4 SDK. This behavior could be changed if -Weverything or
-Winvalid-or-nonexistent-directory options are passed.

  1. Changed

test/Driver/ps4-linker-non-win.c to use -fuse-ld=gold instead of -linker=gold
test/Driver/ps4-linker-win.c to use -fuse-ld=gold instead of -linker=gold
We do not support "-linker" option anymore. Use "-fuse-ld" instead.

  1. test/Driver/ps4-sdk-root.c

Added -Winvalid-or-nonexistent-directory to all the RUN lines to force the warning (this is the only test where we want to have the warnings about missing PS4 SDK directory enabled).

  1. test/Driver/rtti-options.cpp

Removed all the changes. No changes are needed, since the warning is not reported by default anymore.

Origianal patch extracted by Filipe Cabecinhas, me (Katya Romanova), and Pierre Gousseau.

Diff Detail

Repository
rL LLVM

Event Timeline

kromanova updated this revision to Diff 36647.Oct 6 2015, 1:26 PM
kromanova retitled this revision from to Revised Initial patch for PS4 toolchain.
kromanova updated this object.
kromanova added reviewers: filcab, echristo, alexr, probinson.
kromanova set the repository for this revision to rL LLVM.
kromanova added subscribers: silvas, cfe-commits, chapuni and 4 others.

For some reasons the patch file I get from the "download raw diff link" does not contain the diff for the added .keep files? But the .keep files show up in phabricator, so I guess it is a phabricator issue?

Hi Pierre,
I noticed the same issue. When I downloaded a patch from http://reviews.llvm.org/D11279, I had to manually add .keep files to make tests to pass.

It's a Phabricator problem. The diff file that I uploaded to Phabricator contains .keep files, however the diff that is available for download doesn't mention these files. Most likely this happens because .keep files are empty.
Katya.

kromanova updated this revision to Diff 36829.Oct 8 2015, 1:16 AM

Few more changes:

(1) There was a bug, where the PS4 driver didn't add input filename in the call to external assembler.
Filipe fixed this problem in Tools.cpp

(2) A new testcase no-integrated-as.s was added for testing the problem described in (1).

(3) A comment is added in ToolChains.cpp to emphasize that the waning about missing PS4 SDK headers and libs is ignored by default.

filcab accepted this revision.Oct 8 2015, 7:03 AM
filcab edited edge metadata.

LGTM. But let's wait for a !Sony dev to say something too.

This revision is now accepted and ready to land.Oct 8 2015, 7:03 AM

Other than that, LGTM too.

lib/Driver/ToolChains.cpp
4081 ↗(On Diff #36829)

Use llvm::sys::path::append instead of std::string::operator+ for path concatenation here,

4095 ↗(On Diff #36829)

and here,

4105 ↗(On Diff #36829)

and here.

echristo edited edge metadata.Oct 13 2015, 10:38 AM
echristo added a subscriber: echristo.

(Jonathan's review here is fine, don't wait on me. Thanks! :)

-eric

Thank you! I will rebase and commit shortly

Katya.

kromanova updated this revision to Diff 37292.Oct 13 2015, 3:27 PM
kromanova edited edge metadata.

Updated the patch based on Jonathan's comments.
Jonathan, if you have a minute, please review.

jroelofs accepted this revision.Oct 13 2015, 3:40 PM
jroelofs added a reviewer: jroelofs.

One small suggestion. Otherwise, this still LGTM.

lib/Driver/ToolChains.cpp
4078 ↗(On Diff #37292)

It might help to structure this:

const char *EnvValue = getenv("SCE_PS4_SDK_DIR");
if (EnvValue && !llvm::sys::fs::exists(EnvValue))
  getDriver().Diag(clang::diag::warn_drv_ps4_sdk_dir) << EnvValue;

SmallString<512> PS4SDKDir(EnvValue ? EnvValue : getDriver().Dir);
if (!EnvValue) {
  llvm::sys::path::append(PS4SDKDir, "/../../");
}

as:

SmallString<512> PS4SDKDir;
if (const char *EnvValue = getenv("SCE_PS4_SDK_DIR"))
  if (!llvm::sys::fs::exists(EnvValue))
    getDriver().Diag(clang::diag::warn_drv_ps4_sdk_dir) << EnvValue;

  PS4SDKDir = EnvValue;
} else {
  PS4SDKDir = getDriver().Dir;
  llvm::sys::path::append(PS4SDKDir, "/../../");
}
This revision was automatically updated to reflect the committed changes.

Thanks! I just committed the patch.
Katya.

emaste added a subscriber: emaste.Oct 13 2015, 5:12 PM
emaste added inline comments.
cfe/trunk/lib/Driver/ToolChains.cpp
4080

Looks like a missing { here

tfiala added a subscriber: tfiala.Oct 13 2015, 5:25 PM

This change appears to have broken quite a few builders.

Can we get this fixed or reversed out? Thanks!

http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA_build/7935/
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/7334
+ others.

Can we get this fixed or reversed out? Thanks!

It's already reverted.

Ah nice. Thanks!

-Todd

Sorry for causing the trouble with the build. I will try to look later why my partial rebuild didn't catch the issue.
I have fixed the error, rebuilt and ran the tests.

This is a very big patch. Please be understanding, because this patch has a big impact. I apologize in advance if it causes some other problems in the future.

Katya.

Oh no worries, just wanted to make sure wasn't missed, that's all.

-Todd

Hi,

The initial PS4 patch caused a test failure (debug-options.c) on the PS4 bot. I suspect that I know why the problem happens, but I'm not sure what will be the best way to handle it. If someone knows how to fix this test more "elegantly", I would appreciate their advice.

FAIL: Clang :: Driver/debug-options.c (3509 of 24708)

  • TEST 'Clang :: Driver/debug-options.c' FAILED ****

Script:

/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/./bin/clang -### -c -g /home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/Driver/debug-options.c -target x86_64-linux-gnu 2>&1 | /home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/./bin/FileCheck -check-prefix=G /home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-
....

/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/./bin/clang -### -g /home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/Driver/debug-options.c 2>&1 | /home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/./bin/FileCheck -check-prefix=CI /home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/Driver/debug-options.c

Exit Code: 1

Command Output (stderr):

/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/Driver/debug-options.c:139:8: error: expected string not found in input
// CI: "-dwarf-column-info"

^

<stdin>:1:1: note: scanning from here
clang version 3.8.0 (trunk 250262)
^
<stdin>:5:438: note: possible intended match here
"/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang-3.8" "-cc1" "-triple" "x86_64-scei-ps4" "-emit-obj" "-mrelax-all" "-disable-free" "-main-file-name" "debug-options.c" "-mrelocation-model" "pic" "-pic-level" "2" "-mthread-model" "posix" "-mdisable-fp-elim" "-masm-verbose" "-mconstructor-aliases" "-munwind-tables" "-target-cpu" "btver2" "-momit-leaf-frame-pointer" "-debug-info-kind=limited" "-dwarf-version=4" "-backend-option" "-generate-arange-section" "-resource-dir" "/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/../lib/clang/3.8.0" "-fdebug-compilation-dir" "/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/tools/clang/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-stack-protector" "2" "-fdeclspec" "-fobjc-runtime=gnustep" "-fdiagnostics-show-option" "-o" "/tmp/debug-options-1505f5.o" "-x" "c" "/home/buildbot/Buildbot/Slave/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/Driver/debug-options.c"

                                                                                                                                                                                                                                                                                                                                                                     ^

The latest driver patch introduced a change, causing the PS4 driver *not* to have -gcolumn-info enabled by default.

Consequently, this generic line started to fail on the PS4 bot.
// RUN: %clang -### -g %s 2>&1 | FileCheck -check-prefix=CI %s

Does someone know what will be the best way to run the test line for all the platforms, except PS4?

In the patch, we have added a couple of PS4 specific lines to this test, to verify that the behavior on PS4 is correct:

RUN: %clang -### -c %s -g -target x86_64-scei-ps4 2>&1 \
RUN: | FileCheck -check-prefix=NOCI %s

RUN: %clang -### -c %s -g -gcolumn-info -target x86_64-scei-ps4 2>&1 \
RUN: | FileCheck -check-prefix=CI %s

I do not want to make this test XFAIL for PS4 (though I might do it as an interim solution). I would also prefer to avoid duplicating most of the content of this file into a separate ps4-specific file.
Any ideas how to handle this issue "more elegantly"?

If nobody objects, I will mark this test as XFAIL for PS4 for a time being.

Katya.

It was already reverted, but I agree, let's get this fixed first.

Thanks!

-eric