[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In D121838#3543421 , @JamesNagurne 
wrote:

> After some investigation, I found that we did not set LLVM_INCLUDE_TESTS from 
> the top-level project. This seems like an oversight because we build the 
> test-depends (and check-all) regularly.
> After seeing LLVM_INCLUDE_TESTS to ON, the builds worked.
>
> This commit cleaned things up and exposed that mistake.

Sorry, this was a false positive. Building the 'test-depends' target still 
shows the LLVM build system telling the runtimes to build 
'runtimes-test-depends', which is not defined in those ExternalProjects.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121838/new/

https://reviews.llvm.org/D121838

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

After some investigation, I found that we did not set LLVM_INCLUDE_TESTS from 
the top-level project. This seems like an oversight because we build the 
test-depends (and check-all) regularly.
After seeing LLVM_INCLUDE_TESTS to ON, the builds worked.

This commit cleaned things up and exposed that mistake.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121838/new/

https://reviews.llvm.org/D121838

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121838: Generalize "check-all" umbrella targets, use for check-clang-tools

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

Hi Sam, I've got a downstream build system that is failing here and I'm trying 
to figure out why.

After your commit, I get an error during bootstrapped runtime builds:

  05-27 14:41 __main__ INFO [202/796] cd 
/path/to/builddir/runtimes/runtimes-target-bins && cmake --build 
/path/to/builddir/runtimes/runtimes-target-bins/ --target runtimes-test-depends 
--config Release
  05-27 14:41 __main__ INFO ninja: error: unknown target 
'runtimes-test-depends'

We have a mono-project CMakeLists.txt that builds our entire cross-compilation 
toolchain, libraries and all.
The build system uses a bootstrapping strategy, but must separate the builds of 
llvm/clang tools and the runtime builds, so we have LLVM_ENABLE_RUNTIMES set 
but LLVM_BUILD_RUNTIMES=OFF

Why do we do this instead of using the "default build strategy", you may ask? 
Well, the default strategy didn't exist when we set everything up. Wouldn't be 
against trying that if it seems to be the only way to fix the issue I'm seeing.

Specifically, in-order, the actions we perform:

- Build clang/llvm tools (target llvm-build)
- Build in-house tools
- Build in-house C Library (uses just-built tools from above)
- Build libc++, libc++abi, compiler-rt using llvm/runtimes (target 'runtimes'. 
Needs C includes and just-built compiler from previous step)
- Build llvm-test-depends in preparation for check-all, etc. (arbitrarily 
ordered after runtime builds)

We do this by adding llvm as an ExternalProject and then adding step targets 
(llvm-runtimes, llvm-test-depends) that just map to the LLVM equivalent target.

I'll look more deeply into your commit and what's changed in our downstream 
build system, but I wanted to fire this comment off in case you've seen it or 
have a quick way to figure out what might be wrong.
We do have some downstream changes, including an abandoned commit 
(https://reviews.llvm.org/D72947) that could be causing issues. That's also a 
main target of scrutiny.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121838/new/

https://reviews.llvm.org/D121838

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110673: [clang] Don't modify OptRemark if the argument is not relevant

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

You're welcome! Surprising that no downstream clang was running with the old PM 
and assertions...

I guess we better get on the new PM soon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110673/new/

https://reviews.llvm.org/D110673

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110673: [clang] Don't modify OptRemark if the argument is not relevant

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

Perhaps you could reproduce my error -round-trip-args on the -cc1 command line?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110673/new/

https://reviews.llvm.org/D110673

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110673: [clang] Don't modify OptRemark if the argument is not relevant

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

The trigger for the remark I'm seeing is llvm::shouldInline in 
InlineAdvisor.cpp 
((https://github.com/llvm/llvm-project/blob/2240deb9766cc080b351016b0d7f975d7249b113/llvm/lib/Transforms/IPO/Inliner.cpp#L425)
 which is called in a top-level AlwaysInlinerLegacyPass

Clang Release build with LLVM_ENABLE_ASSERTIONS=ON

  %clang_cc1 %s -Rpass=inline -Rno-pass -emit-llvm -o -



- ORE.emit is called, which checks 'enabled()' first
- enabled checks if any of CodeGenOpts.OptimizationRemarkAnalysis, 
OptimizationRemarkMissed, or OptimizationRemark have a valid pattern 
(https://github.com/llvm/llvm-project/blob/2240deb9766cc080b351016b0d7f975d7249b113/clang/lib/CodeGen/CodeGenAction.cpp#L73)
  - Analysis and Missed have valid patterns, while OptimizationRemark does not
  - This coincides with ParseOptimizationRemark returning a result with a valid 
pattern for only "pass-analysis" and "pass-missed"
- The resulting diagnostic is handled by 
clang::BackendConsumer::OptimizationRemarkHandler 
(https://github.com/llvm/llvm-project/blob/2240deb9766cc080b351016b0d7f975d7249b113/clang/lib/CodeGen/CodeGenAction.cpp#L708)
  - The regex is '.*', so the pattern check passes, and then call 
EmitOptimizationMessage
  - Diags (the DiagnosticEngine modified by clang::ProcessWarningOptions) is 
used to report the diagnostic, given a DebugLoc and an ID
  - Diags->getDiagnosticLevel(DiagID, Loc) is Remark, so the diagnostic is 
emitted

-

Nothing here seems out of place. Regarding ProcessWarningOptions:
I've noticed that our tools being validated are doing a -cc1 round trip due to 
having LLVM_ENABLE_ASSERTIONS set on ON 
(https://github.com/llvm/llvm-project/blob/7dffb8b4da530d481977e31f439a92c5f6f2174a/clang/lib/Frontend/CompilerInvocation.cpp#L642)

- -cc1 options are parsed and generate a temporary CompilerInvocation
- The temporary CompilerInvocation is used to generate a second list of 
arguments
- The second list of arguments are parsed into the real CompilerInvocation
- A third list of arguments are generated from the real CompilerInvocation and 
compared against the second set to ensure that they are identical

If I run clang -cc1 with -no-round-trip-args, I no longer see the remarks. 
Adding some prints for each argument list in the round trip, I get (ignoring 
options unrelated to diagnostics):

- Initial (CommandLineArgs): [-Rpass=inline, -Rno-pass]
- GeneratedArgs1: [-Rno-pass, -Rpass-missed=.*, -Rpass-analysis=.*, 
-fdiagnostics-hotness-threshold=0]
- GeneratedArgs2: [-Rno-pass, -Rpass-missed=.*, -Rpass-analysis=.*, 
-fdiagnostics-hotness-threshold=0]

So, the round-trip check passes, but it looks like some internal consistency 
has been violated because parsing the initial CommandLineArgs into a 
CompilerInvocation does not result in erroneous remarks, but re-generating and 
parsing the arguments again leads to a change in behavior.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110673/new/

https://reviews.llvm.org/D110673

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110673: [clang] Don't modify OptRemark if the argument is not relevant

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

I'll take a quick look tomorrow, but the general idea is that on calling 
ParseOptimizationRemark on line 1909 with a -cc1 command line containing 
-Rpass=inline -Rno-pass, Opts.OptimizationRemarkMissed and 
Opts.OptimizationRemarkAnalysis are set to valid patterns (Regex is ".*", 
Pattern is "", and Kind is RK_Missing). This happens around line 1909 in the 
change set.

This configuration makes it into the LLVM backend. When prompted by specific 
calls to llvm::shouldInline (or something similar, can't remember the spelling 
off hand), emits an optimization-missed remark when one of the functions in the 
test's IR is not inlined.

> "...in clang::ProcessWarningOptions() we'll separately look at all -R 
> arguments and turn on/off corresponding diagnostic groups."

Why would -Rno-pass turn off "pass-missed" or "pass-analysis" diagnostic 
groups? That seems counterintuitive. They seem to be different groups, 
considering how they're used in the backend.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110673/new/

https://reviews.llvm.org/D110673

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83174: Teach AttachPreviousImpl to inherit MSInheritanceAttr attribute

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In D83174#2230763 , @aaron.ballman 
wrote:

> Unfortunately, I had to revert the change as it was causing some buildbots to 
> fail. I reverted in 58c305f466d1f78adb10e7295b9bc9fc192a6e09 
> . Some 
> failures include:
>
> http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/20294
> http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/13600
>
> I'm not certain why b.h is not being found by those builders. Do you mind 
> investigating?

I suspect the failures are due to the Windows-style path in the -I option.
I recall lit being able to handle such a thing, but my memory might be a bit 
spotty on its command-line emulation capabilities.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83174/new/

https://reviews.llvm.org/D83174

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68897: [clang][ifs] Avoid assumption of default visibility in InterfaceStubs tests

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

Sadly I'm home from the weekend, so I can't post more diffs for the renaming.
Here's a link to a github search though! I think this would be all the places: 
https://github.com/llvm/llvm-project/search?q=iterface_q=iterface

I nor the rest of my team have the ability to commit, so I need you to do the 
honors. Thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68897/new/

https://reviews.llvm.org/D68897



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63978: Clang Interface Stubs merger plumbing for Driver

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In D63978#1706714 , @plotfi wrote:

> In D63978#1706502 , @JamesNagurne 
> wrote:
>
> > In D63978#1706448 , @plotfi wrote:
> >
> > > In D63978#1706420 , 
> > > @JamesNagurne wrote:
> > >
> > > > Our team maintains a downstream embedded ARM clang distribution and 
> > > > some tests from this commit have begun to fail for us.
> > > >  For a number of these tests, there was a REQUIRES: 
> > > > x86-registered-target at the top, which has now been removed. 
> > > > Specifically, externstatic.c, merge-conflict-test.c, object-float.c, 
> > > > and object.c are failing.
> > > >
> > > > object* tests seem to be based on object.cpp, which had the REQUIRES 
> > > > line, and externstatic.c also had that line prior to the change.
> > > >  I see that @compnerd suggested the removal, but were you certain that 
> > > > these tests would work on clang toolchains for which x86 is not a 
> > > > registered target?
> > > >
> > > > For a failure example, here the output of lit for our toolchain. If you 
> > > > can make sense of it, I'd appreciate input on how we can fix or work 
> > > > around it:
> > > >
> > > >   > /arm-llvm/Release/llvm/bin/clang -c -o - 
> > > > -emit-interface-stubs 
> > > > /llvm-project/clang/test/InterfaceStubs/object.c | 
> > > > /arm-llvm/Release/llvm/bin/FileCheck -check-prefix=CHECK-TAPI 
> > > > /llvm-project/clang/test/InterfaceStubs/object.c
> > > >/llvm-project/clang/test/InterfaceStubs/object.c:5:16: 
> > > > error: CHECK-TAPI: expected string not found in input
> > > >// CHECK-TAPI: data: { Type: Object, Size: 4 }
> > > >   ^
> > > >:1:1: note: scanning from here
> > > >--- !experimental-ifs-v1
> > > >^
> > > >   
> > > >
> > > > And when run without FileCheck, our raw output:
> > > >
> > > >   > /arm-llvm/Release/llvm/bin/clang -c -o - 
> > > > -emit-interface-stubs 
> > > > /llvm-project/clang/test/InterfaceStubs/object.c
> > > >--- !experimental-ifs-v1
> > > >IfsVersion: 1.0
> > > >Triple: thumbv7em-ti-none-eabihf
> > > >ObjectFileFormat: ELF
> > > >Symbols:
> > > >...
> > > >   
> > >
> > >
> > > I am sorry for this James. I can add back the REQUIRES lines for now and 
> > > coordinate with you on making sure your downstream bots are not affected 
> > > again if the REQUIRES are removed again.
> > >  By chance are your bots accessible publicly?
> >
> >
> > Sadly, they are not. It's on our list of things to investigate, but we 
> > don't have the resources to do such a thing quite yet.
> >  I'm looking into the 'arm7*' buildbots to see if they are built similar to 
> > ours so I am not leaving you entirely without something to look at. 
> > However, if it seems to be common knowledge to always include an X86 
> > target, I think I can talk to my team and change up what we do.
> >
> > These buildbots seem to also do LLVM_TARGETS_TO_BUILD=ARM, and then set the 
> > default target triple to a non-x86 triple (the host's)
> >
> > That could point towards us being in error here. I'll investigate things a 
> > little further, and update when I get the chance.
> >  To be clear: this feature should work for any ELF target, correct?
>
>
> Yes, it is designed to work for all ELF targets but at the moment it is still 
> in an early state. I am on the llvm IRC as zer0_ BTW


I'd love to bounce ideas off of people in IRC, but the big mean IT security 
guys say no to any sort of chat programs. It's a real shame.
I found the assumption being missed though, so good news!
Our targets assume hidden visibility by default. After scanning your code (and 
realizing 'interface' is spelled as 'iterface' in a number of places), I 
noticed it was looking only for externally visible decls. After that, I scanned 
out changes and found a sneaky '-fvisibility=hidden' in our toolchain options.

By running all of your tests with '-fvisibility=default', our toolchain passes! 
If you're willing to review/commit the fix upstream, I'm putting up a review 
presently.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63978/new/

https://reviews.llvm.org/D63978



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68897: [clang][ifs] Avoid assumption of default visibility in InterfaceStubs tests

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne created this revision.
JamesNagurne added reviewers: plotfi, snidertm.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In D63978 , tests were added which expect that 
the default visibility is
maintained without an explicit option.

In at least one case, a downstream toolchain had the default
visibility set to 'hidden', which causes these tests to fail.

To avoid such assumptions, we can explicitly pass
-fvisibility=default to the tests that make this assumption.


Repository:
  rC Clang

https://reviews.llvm.org/D68897

Files:
  clang/test/InterfaceStubs/externstatic.c
  clang/test/InterfaceStubs/merge-conflict-test.c
  clang/test/InterfaceStubs/object-float.c
  clang/test/InterfaceStubs/object.c


Index: clang/test/InterfaceStubs/object.c
===
--- clang/test/InterfaceStubs/object.c
+++ clang/test/InterfaceStubs/object.c
@@ -1,6 +1,6 @@
-// RUN: %clang -c -o - -emit-interface-stubs %s | FileCheck 
-check-prefix=CHECK-TAPI %s
-// RUN: %clang -c -o - -emit-interface-stubs %s | FileCheck 
-check-prefix=CHECK-SYMBOLS %s
-// RUN: %clang -c -o - %s | llvm-nm - 2>&1 | FileCheck 
-check-prefix=CHECK-SYMBOLS %s
+// RUN: %clang -fvisibility=default -c -o - -emit-interface-stubs %s | 
FileCheck -check-prefix=CHECK-TAPI %s
+// RUN: %clang -fvisibility=default -c -o - -emit-interface-stubs %s | 
FileCheck -check-prefix=CHECK-SYMBOLS %s
+// RUN: %clang -fvisibility=default -c -o - %s | llvm-nm - 2>&1 | FileCheck 
-check-prefix=CHECK-SYMBOLS %s
 
 // CHECK-TAPI: data: { Type: Object, Size: 4 }
 // CHECK-SYMBOLS: data
Index: clang/test/InterfaceStubs/object-float.c
===
--- clang/test/InterfaceStubs/object-float.c
+++ clang/test/InterfaceStubs/object-float.c
@@ -1,3 +1,3 @@
-// RUN: not %clang -o - -emit-interface-stubs %s %S/object.c 2>&1 | FileCheck 
%s
+// RUN: not %clang -fvisibility=default -o - -emit-interface-stubs %s 
%S/object.c 2>&1 | FileCheck %s
 // CHECK: error: Interface Stub: Size Mismatch
-double data = 42.0;
\ No newline at end of file
+double data = 42.0;
Index: clang/test/InterfaceStubs/merge-conflict-test.c
===
--- clang/test/InterfaceStubs/merge-conflict-test.c
+++ clang/test/InterfaceStubs/merge-conflict-test.c
@@ -1,3 +1,3 @@
-// RUN: not %clang -o libfoo.so -emit-interface-stubs %s %S/driver-test.c 2>&1 
| FileCheck %s
+// RUN: not %clang -fvisibility=default -o libfoo.so -emit-interface-stubs %s 
%S/driver-test.c 2>&1 | FileCheck %s
 // CHECK: error: Interface Stub: Type Mismatch
-int foo;
\ No newline at end of file
+int foo;
Index: clang/test/InterfaceStubs/externstatic.c
===
--- clang/test/InterfaceStubs/externstatic.c
+++ clang/test/InterfaceStubs/externstatic.c
@@ -1,19 +1,19 @@
-// RUN: %clang -c -DSTORAGE="extern" -o - -emit-interface-stubs -std=c99 -xc 
%s | \
+// RUN: %clang -fvisibility=default -c -DSTORAGE="extern" -o - 
-emit-interface-stubs -std=c99 -xc %s | \
 // RUN: FileCheck -check-prefix=CHECK-EXTERN %s
 
-// RUN: %clang -DSTORAGE="extern" -O0 -o - -c -std=c99 \
+// RUN: %clang -fvisibility=default -DSTORAGE="extern" -O0 -o - -c -std=c99 \
 // RUN: -xc %s | llvm-nm - 2>&1 | FileCheck -check-prefix=CHECK-EXTERN %s
 
-// RUN: %clang -c -DSTORAGE="extern" -o - -emit-interface-stubs -std=c99 -xc 
%s | \
+// RUN: %clang -fvisibility=default -c -DSTORAGE="extern" -o - 
-emit-interface-stubs -std=c99 -xc %s | \
 // RUN: FileCheck -check-prefix=CHECK-EXTERN2 %s
 
-// RUN: %clang -DSTORAGE="extern" -O0 -o - -c -std=c99 -xc %s | llvm-nm - 2>&1 
| \
+// RUN: %clang -fvisibility=default -DSTORAGE="extern" -O0 -o - -c -std=c99 
-xc %s | llvm-nm - 2>&1 | \
 // RUN: FileCheck -check-prefix=CHECK-EXTERN2 %s
 
-// RUN: %clang -c -DSTORAGE="static" -o - -emit-interface-stubs -std=c99 -xc 
%s | \
+// RUN: %clang -fvisibility=default -c -DSTORAGE="static" -o - 
-emit-interface-stubs -std=c99 -xc %s | \
 // RUN: FileCheck -check-prefix=CHECK-STATIC %s
 
-// RUN: %clang -DSTORAGE="static" -O0 -o - -c -std=c99 -xc %s | llvm-nm - 2>&1 
| \
+// RUN: %clang -fvisibility=default -DSTORAGE="static" -O0 -o - -c -std=c99 
-xc %s | llvm-nm - 2>&1 | \
 // RUN: FileCheck -check-prefix=CHECK-STATIC %s
 
 // CHECK-EXTERN-NOT: foo


Index: clang/test/InterfaceStubs/object.c
===
--- clang/test/InterfaceStubs/object.c
+++ clang/test/InterfaceStubs/object.c
@@ -1,6 +1,6 @@
-// RUN: %clang -c -o - -emit-interface-stubs %s | FileCheck -check-prefix=CHECK-TAPI %s
-// RUN: %clang -c -o - -emit-interface-stubs %s | FileCheck -check-prefix=CHECK-SYMBOLS %s
-// RUN: %clang -c -o - %s | llvm-nm - 2>&1 | FileCheck -check-prefix=CHECK-SYMBOLS %s
+// RUN: %clang -fvisibility=default -c -o - -emit-interface-stubs %s | FileCheck 

[PATCH] D63978: Clang Interface Stubs merger plumbing for Driver

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In D63978#1706448 , @plotfi wrote:

> In D63978#1706420 , @JamesNagurne 
> wrote:
>
> > Our team maintains a downstream embedded ARM clang distribution and some 
> > tests from this commit have begun to fail for us.
> >  For a number of these tests, there was a REQUIRES: x86-registered-target 
> > at the top, which has now been removed. Specifically, externstatic.c, 
> > merge-conflict-test.c, object-float.c, and object.c are failing.
> >
> > object* tests seem to be based on object.cpp, which had the REQUIRES line, 
> > and externstatic.c also had that line prior to the change.
> >  I see that @compnerd suggested the removal, but were you certain that 
> > these tests would work on clang toolchains for which x86 is not a 
> > registered target?
> >
> > For a failure example, here the output of lit for our toolchain. If you can 
> > make sense of it, I'd appreciate input on how we can fix or work around it:
> >
> >   > /arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs 
> > /llvm-project/clang/test/InterfaceStubs/object.c | 
> > /arm-llvm/Release/llvm/bin/FileCheck -check-prefix=CHECK-TAPI 
> > /llvm-project/clang/test/InterfaceStubs/object.c
> >/llvm-project/clang/test/InterfaceStubs/object.c:5:16: error: 
> > CHECK-TAPI: expected string not found in input
> >// CHECK-TAPI: data: { Type: Object, Size: 4 }
> >   ^
> >:1:1: note: scanning from here
> >--- !experimental-ifs-v1
> >^
> >   
> >
> > And when run without FileCheck, our raw output:
> >
> >   > /arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs 
> > /llvm-project/clang/test/InterfaceStubs/object.c
> >--- !experimental-ifs-v1
> >IfsVersion: 1.0
> >Triple: thumbv7em-ti-none-eabihf
> >ObjectFileFormat: ELF
> >Symbols:
> >...
> >   
>
>
> I am sorry for this James. I can add back the REQUIRES lines for now and 
> coordinate with you on making sure your downstream bots are not affected 
> again if the REQUIRES are removed again.
>  By chance are your bots accessible publicly?


Sadly, they are not. It's on our list of things to investigate, but we don't 
have the resources to do such a thing quite yet.
I'm looking into the 'arm7*' buildbots to see if they are built similar to ours 
so I am not leaving you entirely without something to look at. However, if it 
seems to be common knowledge to always include an X86 target, I think I can 
talk to my team and change up what we do.

These buildbots seem to also do LLVM_TARGETS_TO_BUILD=ARM, and then set the 
default target triple to a non-x86 triple (the host's)

That could point towards us being in error here. I'll investigate things a 
little further, and update when I get the chance.
To be clear: this feature should work for any ELF target, correct?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63978/new/

https://reviews.llvm.org/D63978



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63978: Clang Interface Stubs merger plumbing for Driver

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

Our team maintains a downstream embedded ARM clang distribution and some tests 
from this commit have begun to fail for us.
For a number of these tests, there was a REQUIRES: x86-registered-target at the 
top, which has now been removed. Specifically, externstatic.c, 
merge-conflict-test.c, object-float.c, and object.c are failing.

object* tests seem to be based on object.cpp, which had the REQUIRES line, and 
externstatic.c also had that line prior to the change.
I see that @compnerd suggested the removal, but were you certain that these 
tests would work on clang toolchains for which x86 is not a registered target?

For a failure example, here the output of lit for our toolchain. If you can 
make sense of it, I'd appreciate input on how we can fix or work around it:

  > /arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs 
/llvm-project/clang/test/InterfaceStubs/object.c | 
/arm-llvm/Release/llvm/bin/FileCheck -check-prefix=CHECK-TAPI 
/llvm-project/clang/test/InterfaceStubs/object.c
  /llvm-project/clang/test/InterfaceStubs/object.c:5:16: error: 
CHECK-TAPI: expected string not found in input
  // CHECK-TAPI: data: { Type: Object, Size: 4 }
 ^
  :1:1: note: scanning from here
  --- !experimental-ifs-v1
  ^

And when run without FileCheck, our raw output:

  > /arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs 
/llvm-project/clang/test/InterfaceStubs/object.c
  --- !experimental-ifs-v1
  IfsVersion: 1.0
  Triple: thumbv7em-ti-none-eabihf
  ObjectFileFormat: ELF
  Symbols:
  ...


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63978/new/

https://reviews.llvm.org/D63978



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In D65907#1645474 , @arphaman wrote:

> In D65907#1643800 , @JamesNagurne 
> wrote:
>
> > In D65907#1643650 , @arphaman 
> > wrote:
> >
> > > No the windows test failure was different, there were no Deps at all. I'm 
> > > currently investigating it on a windows VM.
> > >
> > > @JamesNagurne I think there's some issue with the working directory, 
> > > which is not added in your case. Which platform are you running your 
> > > build/test on? Which cmake options are you using?
> >
> >
> > I apologize for not giving such information in the first reply. 
> > Unfortunately this isn't an easy remote reproduction, as our ToolChain and 
> > some integral changes aren't upstreamed. This is an embedded ARM 
> > cross-compiled on Linux. Might be able to reproduce with arm-none-none-eabi.
> >  LLVM is built as an external project. Looking at the build system, it 
> > looks like we have the CMAKE_ARGS:
> >
> >  
> >   -DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
> >   -DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
> >   -DLLVM_TARGETS_TO_BUILD=ARM
> >   -DCMAKE_BUILD_TYPE=Release
> >   -DCMAKE_CXX_COMPILER=clang++
> >   -DCMAKE_C_COMPILER=clang
> >   -DLLVM_USE_LINKER=gold
> >   -GNinja
> >   
> >
> > Nothing suspicious, except maybe the default triple and ARM target.
> >  Looking at our (not upstream) toolchain file, we do have some RTLibs, 
> > LibInternal, libcxx, and System include changes, along with a 
> > -nostdsysteminc to avoid pulling host includes into our cross compiler. 
> > However, none of this should affect general "#include" behavior, correct?
> >  Another glance at your changes don't seem to affect any target-specific 
> > handling either, at least directly.
>
>
> Yes, I don't see anything in your changes that is an obvious thing to blame.
>
> > I might have to just bite the bullet and drop into the test with a debugger.
>
> I fixed the Windows issue, and it was completely unrelated to your issue. It 
> looks like the FileManager isn't constructing absolute paths when accessing 
> the files, which is somewhat unexpected. It would be useful to debug 
> invocations of `getFileEntryRef`, to see if the `InterndFilename` in the 
> function is getting changed into an absolute path or not.


We found and fixed the issue on our end. Turns out we had a nefarious little 
piece of code in AddClangSystemIncludeArgs:

  if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) {
SmallString<128> Dir(getDriver().SysRoot);
llvm::sys::path::append(Dir, "include");
addSystemInclude(DriverArgs, CC1Args, Dir.str());
  }

Turns out that, as a cross compiler, we had set SysRoot to the empty string. 
So, we were adding "-internal-isystem" "include" to the cc1 arguments, which 
somehow caused the test to fail. I don't know precisely why it exposes issues 
in your test, but the code is clearly busted, so we've removed it.

I appreciate the responses as we've worked through the problems on our end!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In D65907#1643650 , @arphaman wrote:

> No the windows test failure was different, there were no Deps at all. I'm 
> currently investigating it on a windows VM.
>
> @JamesNagurne I think there's some issue with the working directory, which is 
> not added in your case. Which platform are you running your build/test on? 
> Which cmake options are you using?


I apologize for not giving such information in the first reply. Unfortunately 
this isn't an easy remote reproduction, as our ToolChain and some integral 
changes aren't upstreamed. This is an embedded ARM cross-compiled on Linux. 
Might be able to reproduce with arm-none-none-eabi.
LLVM is built as an external project. Looking at the build system, it looks 
like we have the CMAKE_ARGS:

  -DLLVM_DEFAULT_TARGET_TRIPLE=arm-ti-none-eabi
  -DLLVM_EXTERNAL_CLANG_SOURCE_DIR=${CMAKE_SOURCE_DIR}/llvm-project/clang
  -DLLVM_TARGETS_TO_BUILD=ARM
  -DCMAKE_BUILD_TYPE=Release
  -DCMAKE_CXX_COMPILER=clang++
  -DCMAKE_C_COMPILER=clang
  -DLLVM_USE_LINKER=gold
  -GNinja

Nothing suspicious, except maybe the default triple and ARM target.
Looking at our (not upstream) toolchain file, we do have some RTLibs, 
LibInternal, libcxx, and System include changes, along with a -nostdsysteminc 
to avoid pulling host includes into our cross compiler. However, none of this 
should affect general "#include" behavior, correct?
Another glance at your changes don't seem to affect any target-specific 
handling either, at least directly.

I might have to just bite the bullet and drop into the test with a debugger.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65907: Introduce FileEntryRef and use it when handling includes to report correct dependencies when the FileManager is reused across invocations

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.
Herald added a subscriber: ributzka.

@arphaman you disabled this test on Windows, but did not specify exactly how it 
fails.
My team works on an embedded ARM compiler (most similar to arm-none-eabi), and 
we're now seeing failures from DependencyScannerTest. I can't find a buildbot 
failure for this test so I can't cross-reference to see if we have the same 
issue.

Does this failure look similar to what you saw on Windows, or could it be an 
option we're adding as part of the Compilation setup?

  [ RUN  ] DependencyScanner.ScanDepsReuseFilemanager
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:100: Failure
Expected: Deps[1]
Which is: "symlink.h"
  To be equal to: "/root/symlink.h"
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:101: Failure
Expected: Deps[2]
Which is: "header.h"
  To be equal to: "/root/header.h"
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:113: Failure
Expected: Deps[1]
Which is: "symlink.h"
  To be equal to: "/root/symlink.h"
  .../clang/unittests/Tooling/DependencyScannerTest.cpp:114: Failure
Expected: Deps[2]
Which is: "header.h"
  To be equal to: "/root/header.h"
  [  FAILED  ] DependencyScanner.ScanDepsReuseFilemanager (5 ms)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65907/new/

https://reviews.llvm.org/D65907



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60943: Delay diagnosing asm constraints that require immediates until after inlining

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In case you haven't seen, this commit breaks non-x86 build bots due to the 
combination of '-triple x86_64*' and '-S'. Some tests that use this target are 
only looking for AST dumps, and do not actually require such a target. This is 
not one of those tests, as it's inspecting assembly.
See clang/test/CodeGen/addrsig.c to see how that is handled (via REQUIRES: 
x86-registered-target).

Oddly enough, other tests that use -triple x86_64* and only inspect the AST 
don't require the registered target.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60943/new/

https://reviews.llvm.org/D60943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64653: clang/test/Driver/fsanitize.c: Fix -fsanitize=vptr using default target

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

In D64653#1584155 , @MaskRay wrote:

> I committed this for you in rL365981  (it 
> may have broken a Windows build for a long time. I wanted to fix it soon..). 
> Thanks!
>
> > any platform that runs this test will fail with the error:
>
> Not-too-ancient Darwin, FreeBSD, Linux, NetBSD, OpenBSD, Solaris, etc support 
> this, but notable exception is Windows:
>
>   % clang -target x86_64-windows -fsanitize=vptr -fno-rtti fsanitize.c '-###'
>   clang-9: error: unsupported option '-fsanitize=vptr' for target 
> 'x86_64-unknown-windows-msvc'
>


Yes, my comment was rather misguided here. I didn't mean to say "All 
platforms", more like "All platforms that don't support -fsanitize=vptr". 
Specifically, this was affecting our embedded arm target.
Thanks for getting this in!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64653/new/

https://reviews.llvm.org/D64653



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64653: clang/test/Driver/fsanitize.c: Fix -fsanitize=vptr using default target

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne created this revision.
JamesNagurne added a reviewer: MaskRay.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

test/Driver/fsanitize.c: Fix -fsanitize=vptr using default target

In revision rL365872 , a line containing 
'-target x86_64-linux-gnu' along
with '-fsanitize=vptr' was modified to only contain the -fsanitize option.

The default implementation of getSupportedSanitizers isn't able to turn
on the vptr sanitizer, and thus, any platform that runs this test will
fail with the error:

  clang: error: unsupported option '-fsanitize=vptr' for target ''


Repository:
  rC Clang

https://reviews.llvm.org/D64653

Files:
  clang/test/Driver/fsanitize.c


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -97,7 +97,7 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=vptr 
-fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-VPTR-TRAP-UNDEF
 // CHECK-VPTR-TRAP-UNDEF: error: invalid argument '-fsanitize=vptr' not 
allowed with '-fsanitize-trap=undefined'
 
-// RUN: %clang -fsanitize=vptr -fno-rtti %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-VPTR-NO-RTTI
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=vptr -fno-rtti %s -### 2>&1 
| FileCheck %s --check-prefix=CHECK-VPTR-NO-RTTI
 // CHECK-VPTR-NO-RTTI: '-fsanitize=vptr' not allowed with '-fno-rtti'
 
 // RUN: %clang -fsanitize=undefined -fno-rtti %s -### 2>&1 | FileCheck %s 
--check-prefix=CHECK-UNDEFINED-NO-RTTI


Index: clang/test/Driver/fsanitize.c
===
--- clang/test/Driver/fsanitize.c
+++ clang/test/Driver/fsanitize.c
@@ -97,7 +97,7 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=vptr -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-TRAP-UNDEF
 // CHECK-VPTR-TRAP-UNDEF: error: invalid argument '-fsanitize=vptr' not allowed with '-fsanitize-trap=undefined'
 
-// RUN: %clang -fsanitize=vptr -fno-rtti %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-NO-RTTI
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=vptr -fno-rtti %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-VPTR-NO-RTTI
 // CHECK-VPTR-NO-RTTI: '-fsanitize=vptr' not allowed with '-fno-rtti'
 
 // RUN: %clang -fsanitize=undefined -fno-rtti %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-NO-RTTI
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.

[James Nagurne via Phabricator via cfe-commits]

JamesNagurne added a comment.

Hi Simon et. al., I'm working on a downstream ARM toolchain and have 
downstreamed this change into our codebase.
We saw that you've fixed the -mfpu=none issue and have taken that as well, but 
are still running into some issues.

Prior to your change, the optionset "-mcpu=cortex-m4 -mfloat-abi=hard" created 
the following target features in the LLVM IR coming out of clang:
"target-features"="+armv7e-m,+d16,+dsp,+fp-only-sp,+hwdiv,+thumb-mode,+vfp4,-crc,-dotprod,-fp16fml,-hwdiv-arm,-ras"

In specific, note the +d16, +fp-only-sp.

After your changes, we're not seeing the associated -d32, -fp64 target features:
"target-cpu"="cortex-m4" 
"target-features"="+armv7e-m,+dsp,+hwdiv,+thumb-mode,+vfp4,-crc,-dotprod,-fp16fml,-hwdiv-arm,-ras"

As a result, we are getting linktime failures between our libraries, which use 
"-mcpu=cortex-m4 -mfloat-abi=hard", and our tests, which specifically call out 
the FPU version being used.
An interesting note is that the target machine table has the following:

  def : ProcessorModel<"cortex-m4", CortexM4Model,[ARMv7em,
   FeatureVFP4_D16_SP,
   
FeaturePrefLoopAlign32,
   FeatureHasSlowFPVMLx,
   FeatureUseMISched,
   FeatureUseAA,
   
FeatureHasNoBranchPredictor]>;

Would this not mean that we'd expect vfp4-d16-sp when not otherwise specified? 
If not, then what is the expected behavior of -mfloat-abi=hard in the absence 
of an -mfpu specification?

Thanks,
J.B. Nagurne
Texas Instruments
Code Generation


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60691/new/

https://reviews.llvm.org/D60691



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits