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

James Nagurne via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 11 16:24:00 PDT 2019


JamesNagurne added a comment.

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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63978





More information about the llvm-commits mailing list