In D53005#2064021, @fpichet wrote:

I am working on an out of tree target and this would be useful.
So what is the status of this patch?

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?

In rG04a6ea5d77e7613a5e1398ddf2a0fcb4e1cea41c#942027, @aemerson wrote:

In rG04a6ea5d77e7613a5e1398ddf2a0fcb4e1cea41c#942000, @JamesNagurne wrote:

This commit is causing build failures in my downstream ARM repository, as well as at least one buildbot (http://lab.llvm.org:8011/builders/clang-cmake-x86_64-avx2-linux)

The errors seem to stem from the std::tuple implementation. Could it be that the buildbot and my downstream are using older, unsupported library implementations?

Could be, but I'm not certain. In any case, I've just changed them to use std::make_tuple now instead in ed353445248.

This commit is causing build failures in my downstream ARM repository, as well as at least one buildbot (http://lab.llvm.org:8011/builders/clang-cmake-x86_64-avx2-linux)

Abandoning in lieu of fix in D85215

Moved test to X86 subdirectory

Either of these options sounds good to me.

I apologize for the constant early changes. I forget how phabricator catches and marks things as being referenced, so I was trying to get it to work.
This is a patch fix for D69384, which has broken ARM build bots. I present this as a band-aid since this feature and its test seems to be rather generic.

In D69384#2190796, @ostannard wrote:

This test case has been failing on many of the ARM and AArch64 bots all weekend, e.g. http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/19408, would you mind looking into this?

Here's the log of one of the failures (from http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/19408/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Acoldentrycount.ll):

******************** TEST 'LLVM :: Transforms/HotColdSplit/coldentrycount.ll' FAILED ********************
Script:
--
: 'RUN: at line 4';   /home/tcwg-buildslave/worker/clang-cmake-armv7-quick/stage1/bin/opt -hotcoldsplit -hotcoldsplit-threshold=0 -codegenprepare -S < /home/tcwg-buildslave/worker/clang-cmake-armv7-quick/llvm/llvm/test/Transforms/HotColdSplit/coldentrycount.ll | /home/tcwg-buildslave/worker/clang-cmake-armv7-quick/stage1/bin/FileCheck /home/tcwg-buildslave/worker/clang-cmake-armv7-quick/llvm/llvm/test/Transforms/HotColdSplit/coldentrycount.ll
--
Exit Code: 2

Command Output (stderr):
--
LLVM ERROR: Trying to construct TargetPassConfig without a target machine. Scheduling a CodeGen pass without a target triple set?
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/tcwg-buildslave/worker/clang-cmake-armv7-quick/stage1/bin/opt -hotcoldsplit -hotcoldsplit-threshold=0 -codegenprepare -S 
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/tcwg-buildslave/worker/clang-cmake-armv7-quick/stage1/bin/FileCheck /home/tcwg-buildslave/worker/clang-cmake-armv7-quick/llvm/llvm/test/Transforms/HotColdSplit/coldentrycount.ll

--

********************

In D72947#2047140, @beanz wrote:

Sorry for being late to the party on this.

There's really only a handful of variables in LLVMConfig.cmake that can actually be different between LLVM and the project using LLVM. Might it make sense to just handle those as special? We already do handle LLVM_PTHREAD_LIB specially for exactly the same reason you describe in the description.

The problem here is the method we reset the variables in the cache back to their cached value.

Actually, it's very fortuitous that you reminded of this review today.

When @phosek gets back to it, one of you would need to commit it on my behalf.

Unfortunately I don't have commit access.
Could one of you get this onto the upstream?

This commit broke our validations. Specifically, the tests in llvm/test/tools/opt-viewer

@bkramer committed a fix for a crash in LegalizeFloatTypes where this operator appeared in SoftPromoteHalfResult

Thanks @phosek

In D72947#1850273, @phosek wrote:

This looks reasonable to me, @smeenai & @beanz do you see any problem with this?

In D72952#1841048, @mstorsjo wrote:

In D72952#1840841, @mstorsjo wrote:

In D72952#1840794, @mstorsjo wrote:

This broke my build, for both mingw and linux, with both cmake 3.10.2 and 3.16.2 - the assembly files don't get built at all. Reproducible on linux with a plain "cmake .." (in llvm-project/libunwind/build) without further args.

Is some custom build config needed for this to work?

Ok, so it seems this works when built via the main llvm cmakefile, but not standalone. So I guess the libunwind cmakefiles needs some extras for hooking this up, that are included only when built standalone.

I fixed this in https://reviews.llvm.org/rG0e0c65264aeb6f66b6f711884c55cdbf66d975f6, but building assembly files as assembly instead of C breaks building for mingw, for which I sent an additional patch at D73436, essentially conditionally reverting this change for mingw targets.

Moved option into a block guarded by COMPILER_RT_STANDLONE_BUILD, to indicate that it is an option that is available when not utilizing a linked LLVM toolchain and its configuration.

Updated to reflect master -> diff 2, rather than diff 1 -> diff 2

In D72952#1827668, @smeenai wrote:

The diff here looks weird ... it looks like a diff against your previous diff instead of a diff against master? (Phabricator says the line you're deleting has LANGUAGE ASM instead of LANGUAGE C, as it does on master.)

Confirmed that cmake correctly identifies '.S' as ASM, and uses CMAKE_ASM_FLAGS

@phosek or really any other committer, could we get this onto master?

Sadly, I do not have commit access. Could you do the honors @phosek ?

In D71570#1792087, @phosek wrote:

I'm fine with this change in general, but I'm concerned that no matter what order we choose, this might break users who rely on the opposite order. Maybe it'd be better to modify the logic to skip the second loop if the first loop matches, that way we wouldn't rely on any particular order.

This foreach loop goes over each variable, yes?
If so, then I'm not sure this change does what is required.

This commit has broken a few build bots.

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&unscoped_q=iterface

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:

> <WORKDIR>/arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs <WORKDIR>/llvm-project/clang/test/InterfaceStubs/object.c | <WORKDIR>/arm-llvm/Release/llvm/bin/FileCheck -check-prefix=CHECK-TAPI <WORKDIR>/llvm-project/clang/test/InterfaceStubs/object.c
<WORKDIR>/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 }
               ^
<stdin>:1:1: note: scanning from here
--- !experimental-ifs-v1
^

And when run without FileCheck, our raw output:

> <WORKDIR>/arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs <WORKDIR>/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

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:

> <WORKDIR>/arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs <WORKDIR>/llvm-project/clang/test/InterfaceStubs/object.c | <WORKDIR>/arm-llvm/Release/llvm/bin/FileCheck -check-prefix=CHECK-TAPI <WORKDIR>/llvm-project/clang/test/InterfaceStubs/object.c
<WORKDIR>/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 }
               ^
<stdin>:1:1: note: scanning from here
--- !experimental-ifs-v1
^

And when run without FileCheck, our raw output:

> <WORKDIR>/arm-llvm/Release/llvm/bin/clang -c -o - -emit-interface-stubs <WORKDIR>/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?

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.

Ah! My apologies. I had not pulled anything new down, as our process tends to hard-stop until an issue is resolved (something I'm thinking needs to be a little more flexible).

As mentioned in an inline comment, the test only works if the sizeof(int) != sizeof(int *) and sizeof(unsigned long long) == sizeof(int)

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.

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?

@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.

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).

Our team independently applied and validated this change for our ARM cross compiler running on darwin, and we shared the same failure modes as the darwin compiler. I feel that this is sufficient evidence that the bug is related to the host, not the target.

In D64802#1602758, @gbreynoo wrote:

Hi James,

From my understanding these has been resolving correctly as XFAIL's on upstream Darwin test machines and XFAIL: darwin is used in a few other lit tests. I do not know what cross compiling is tested on the build bots however.

Firstly, does use of XFAIL: system-darwin instead of XFAIL: darwin fix this issue?

Second can you run any of the llvm tests below on your machine and get the expected result? They all use XFAIL: darwin:

• test\DebugInfo\Generic\empty.ll
• test\DebugInfo\Generic\gmlt.test
• test\ExecutionEngine\MCJIT\test-global-ctors.ll
• test\ExecutionEngine\OrcMCJIT\test-global-ctors.ll

Finally I believe lit just uses the standard python commands to determine the platform, do the commands below give unexpected output?

import sys
import platform
print(platform.system())
print(sys.platform)

Hopefully this will make finding the cause of the issue easier. If anyone else has suggestions I would be happy to hear them.

Thanks

Our team maintains a downstream embedded ARM cross compiler, and these tests are failing on our Mac validations.
The key problem here I believe is the 'XFAIL: darwin' doesn't seem to work how you expect it to, and I'm not aware of a good alternative that I can suggest. Hopefully you or someone else has one.

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'

In rL365872#670346, @RKSimon wrote:

@MaskRay This is causing EXPENSIVE_CHECKS failures - please can you investigate ?

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/18594/steps/test-check-all/logs/stdio

Oh dear... I apologize for that lapse of concentration. You are completely correct, someone had modified the signature in a prior commit, and I hadn't gone back far enough to see it.
Thank you for the response.

Maybe someone can enlighten me as to why the build bots aren't tripping up on this one, but our group is running into this when we pull this commit from the upstream:

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.

Closing this, as a fix has already been committed and validated on our end.

I saw that too. Not really too thrilled with the fix itself, since it further fragments the source with directives, but it works.
I do have another question for @EricWF regarding the inclusion of unistd.h, but I should probably take that to a separate forum (though I've commented on the original review that added the inclusion).

This is an example of a solution, but I'm not married to the mechanisms by which the results are achieved:

@EricWF is there an implicit expectation that all implementations based off libcxx/libcxxabi supports POSIX?
Our downstream ARM toolchain does not supply a unistd.h as part of its custom libc, and it has not been a problem until this revision.

This commit has broken in our downstream tooling, which doesn't use the Microsoft ABI nor (I believe) has modified mangling in any way.