This is an archive of the discontinued LLVM Phabricator instance.

Differential D99901

[Driver][test] Test intended target only
ClosedPublic

Authored by jsji on Apr 5 2021, 2:43 PM.

Details

Reviewers
MaskRay
Commits
rGa723310b419b: [Driver][test] Test intended target only
Summary

6fe7de90b9e4e466a5c2baadafd5f72d3203651d changed GNU toolchain,
and added new RUN line to test expected behavior.

The change is for GNU toolchain only, so this will fail other toolchain,
eg: AIX.

Update the test with -target to test GNU tool chain only.

Diff Detail

Event Timeline

jsji requested review of this revision.Apr 5 2021, 2:43 PM
jsji created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2021, 2:43 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Harbormaster completed remote builds in B97175: Diff 335332.Apr 5 2021, 3:55 PM
MaskRay added inline comments.Apr 5 2021, 11:01 PM
clang/test/Driver/nostdincxx.cpp
4

error: unknown target triple 'unknown-unknown-hurd-gnu', please use -triple or -arch

Perhaps this should UNSUPPORTED aix if aix does not support -nostdinc++.

jsji updated this revision to Diff 335499.Apr 6 2021, 6:39 AM

Use -arch instead

clang/test/Driver/nostdincxx.cpp
4

No, AIX does support -nostdinc++, we don't want to unsupport this whole test

jsji updated this revision to Diff 335500.Apr 6 2021, 6:40 AM

Remove typo.

Harbormaster completed remote builds in B97297: Diff 335499.Apr 6 2021, 7:14 AM
Harbormaster completed remote builds in B97298: Diff 335500.Apr 6 2021, 7:20 AM
MaskRay accepted this revision.Apr 7 2021, 11:13 AM
This revision is now accepted and ready to land.Apr 7 2021, 11:13 AM
Closed by commit rGa723310b419b: [Driver][test] Test intended target only (authored by jsji). · Explain WhyApr 7 2021, 1:08 PM
This revision was automatically updated to reflect the committed changes.
jsji added a commit: rGa723310b419b: [Driver][test] Test intended target only.
ted added a subscriber: ted.EditedMay 28 2021, 7:31 AM

Why would we not want to support this on other targets, like Arm? Fixes for AIX shouldn't impact other things.

jsji added a comment.May 28 2021, 7:45 AM
In D99901#2787034, @ted wrote:

Why would we not want to support this on other targets, like Arm? Fixes for AIX shouldn't impact other things.

Feel free to update the triple to include arm , as long as the tests focus on gnu tool chain only.

hubert.reinterpretcast added a subscriber: hubert.reinterpretcast.May 28 2021, 8:25 AM
hubert.reinterpretcast added inline comments.
clang/test/Driver/nostdincxx.cpp
4

I believe the original fix was meant to be generally applicable but unintentionally applied in only a limited fashion. Ideally, the test would have been left as-is and the various toolchains changed similarly.

That is:

$ clang -nostdinc -nostdinc++ -xc++ /dev/null -fsyntax-only
clang-13: warning: argument unused during compilation: '-nostdinc++' [-Wunused-command-line-argument]

should not happen. I guess a way forward is to first make the change for AIX (and update the test to check on AIX) and then to commit a separate NFCI commit to apply the test broadly to see if anyone finds the test to be failing.

thakis mentioned this in rG23f256f2b198: [clang] Fix typo in test from a723310b4.Sep 10 2021, 11:17 AM
thakis added a subscriber: thakis.Sep 10 2021, 11:17 AM
thakis added inline comments.
clang/test/Driver/nostdincxx.cpp
4

FYI: %clangxx is a driver invocation, not a cc1 invocation. The driver flag is --target=, not -triple.

This test, when run manually, produces:

bin/clang  -triple x86_64-unknown-unknown-gnu -fsyntax-only -nostdinc -nostdinc++ /Users/thakis/src/llvm-project/clang/test/Driver/nostdincxx.cpp
clang: error: unknown argument: '-triple'
clang: error: no such file or directory: 'x86_64-unknown-unknown-gnu'

That passes, but probably not for the reasons you want :) I fixed this in 23f256f2b198.

Revision Contents

PathSize
clang/
test/
Driver/
2 lines

Diff 335500

clang/test/Driver/nostdincxx.cpp

// RUN: not %clangxx -nostdinc %s 2>&1 | FileCheck %s // RUN: not %clangxx -nostdinc %s 2>&1 | FileCheck %s
// RUN: not %clangxx -nostdinc++ %s 2>&1 | FileCheck %s // RUN: not %clangxx -nostdinc++ %s 2>&1 | FileCheck %s
// RUN: not %clangxx -nostdlibinc %s 2>&1 | FileCheck %s // RUN: not %clangxx -nostdlibinc %s 2>&1 | FileCheck %s
// RUN: not %clangxx -fsyntax-only -nostdinc -nostdinc++ %s 2>&1 | FileCheck /dev/null --implicit-check-not=-Wunused-command-line-argument // RUN: not %clangxx -triple x86_64-unknown-unknown-gnu -fsyntax-only -nostdinc -nostdinc++ %s 2>&1 | FileCheck /dev/null --implicit-check-not=-Wunused-command-line-argument
MaskRayUnsubmitted
Not Done
ReplyInline Actions

error: unknown target triple 'unknown-unknown-hurd-gnu', please use -triple or -arch

Perhaps this should UNSUPPORTED aix if aix does not support -nostdinc++.

MaskRay: `error: unknown target triple 'unknown-unknown-hurd-gnu', please use -triple or -arch` Perhaps…
jsjiAuthorUnsubmitted
Done
ReplyInline Actions

No, AIX does support -nostdinc++, we don't want to unsupport this whole test

jsji: No, AIX does support `-nostdinc++`, we don't want to unsupport this whole test
hubert.reinterpretcastUnsubmitted
Not Done
ReplyInline Actions

I believe the original fix was meant to be generally applicable but unintentionally applied in only a limited fashion. Ideally, the test would have been left as-is and the various toolchains changed similarly.

That is:

$ clang -nostdinc -nostdinc++ -xc++ /dev/null -fsyntax-only
clang-13: warning: argument unused during compilation: '-nostdinc++' [-Wunused-command-line-argument]

should not happen. I guess a way forward is to first make the change for AIX (and update the test to check on AIX) and then to commit a separate NFCI commit to apply the test broadly to see if anyone finds the test to be failing.

hubert.reinterpretcast: I believe the original fix was meant to be generally applicable but unintentionally applied in…
thakisUnsubmitted
Not Done
ReplyInline Actions

FYI: %clangxx is a driver invocation, not a cc1 invocation. The driver flag is --target=, not -triple.

This test, when run manually, produces:

bin/clang  -triple x86_64-unknown-unknown-gnu -fsyntax-only -nostdinc -nostdinc++ /Users/thakis/src/llvm-project/clang/test/Driver/nostdincxx.cpp
clang: error: unknown argument: '-triple'
clang: error: no such file or directory: 'x86_64-unknown-unknown-gnu'

That passes, but probably not for the reasons you want :) I fixed this in 23f256f2b198.

thakis: FYI: `%clangxx` is a driver invocation, not a cc1 invocation. The driver flag is `--target=`…
// CHECK: file not found // CHECK: file not found
#include <vector> #include <vector>
// MSVC and PS4 have C++ headers in the same directory as C headers. // MSVC and PS4 have C++ headers in the same directory as C headers.
// UNSUPPORTED: ms-sdk, ps4 // UNSUPPORTED: ms-sdk, ps4