plotfi added a comment. In D63978#1706764 <https://reviews.llvm.org/D63978#1706764>, @JamesNagurne wrote:
> In D63978#1706714 <https://reviews.llvm.org/D63978#1706714>, @plotfi wrote: > > > In D63978#1706502 <https://reviews.llvm.org/D63978#1706502>, @JamesNagurne > > wrote: > > > > > In D63978#1706448 <https://reviews.llvm.org/D63978#1706448>, @plotfi > > > wrote: > > > > > > > In D63978#1706420 <https://reviews.llvm.org/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 > > > 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. Fan-freakin-tastic! I can help review, and can you include the places where I misspelled things?? 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
