Page MenuHomePhabricator

Clang Interface Stubs merger plumbing for Driver
ClosedPublic

Authored by plotfi on Jun 30 2019, 10:11 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2019, 10:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
plotfi updated this revision to Diff 207224.Jun 30 2019, 10:13 AM

had some accidentally not-removed stray print output and/or expletives, apologies.

plotfi updated this revision to Diff 214924.Aug 13 2019, 2:39 PM
plotfi edited the summary of this revision. (Show Details)

Updated. Much better cleaner implementation.

plotfi retitled this revision from Very early work on interface stub merger plumbing. to Clang Interface Stubs merger plumbing for Driver.Aug 13 2019, 2:40 PM

Tests?

clang/lib/Driver/Driver.cpp
3420 ↗(On Diff #214924)

This isn't really a link action ...

3423 ↗(On Diff #214924)

I don't understand why the offload bundler is part of the interface merge phase.

clang/lib/Driver/ToolChains/InterfaceStubs.cpp
11 ↗(On Diff #214924)

Why do you need GCC_INSTALL_PREFIX? GCC doesn't have interface stubs yet AFAIK.

34 ↗(On Diff #214924)

debugging left overs?

36 ↗(On Diff #214924)

Hmm, this seems wrong :-). You should search for it relative to the toolchain and then fallback.

clang/lib/Driver/Types.cpp
328 ↗(On Diff #214924)

How does -c -emit-interface-stubs and multiple input play together?

plotfi updated this revision to Diff 219211.Sep 6 2019, 6:17 PM
plotfi marked 2 inline comments as done.
plotfi added a reviewer: cishida.

Adding better wiring up to llvm-ifs

plotfi marked 2 inline comments as done.Sep 6 2019, 6:19 PM
plotfi updated this revision to Diff 219226.Sep 7 2019, 12:49 AM
plotfi marked an inline comment as done.
cishida added inline comments.Sep 7 2019, 6:03 PM
clang/lib/Driver/ToolChains/Clang.cpp
3688 ↗(On Diff #219226)

nit: no need to check ArgStr if all outcomes are the same

compnerd added inline comments.Sep 13 2019, 4:28 PM
clang/include/clang/Driver/Phases.h
24 ↗(On Diff #219226)

Trailing comma please

clang/lib/Driver/Driver.cpp
3341 ↗(On Diff #219226)

Bleh, this really should be something that we should be able to determine based on the input type.

clang/lib/Driver/ToolChains/Clang.cpp
3688 ↗(On Diff #219226)

Why have this switch and check below at all? I think that it should just be an assertion that the string is equal to that value at the very most. L3690-L3700 is dead code. Please remove, along with L3685-L3688.

clang/lib/Driver/ToolChains/InterfaceStubs.cpp
15 ↗(On Diff #219226)

Why not:

namespace tools {
namespace ifstool {
26 ↗(On Diff #219226)

A ternary might be easier to read

29 ↗(On Diff #219226)

const auto &Input?

clang/lib/Frontend/CompilerInvocation.cpp
1741 ↗(On Diff #219226)

This seems entirely unnecessary, the default and case list is identical.

plotfi updated this revision to Diff 221209.Sep 21 2019, 11:53 PM
plotfi marked 7 inline comments as done.Sep 22 2019, 12:00 AM
plotfi added inline comments.
clang/lib/Driver/Types.cpp
328 ↗(On Diff #214924)

Dropped the code for -c. For now, to get the straight up ifs I think -cc1 can be sufficient.

There are some hairy things regarding getting the correct InputType from Driver::BuildInputs that need to be sorted first.

plotfi updated this revision to Diff 221210.Sep 22 2019, 12:00 AM
plotfi marked an inline comment as done.

fixed comment.

plotfi marked an inline comment as done.Sep 22 2019, 12:27 AM
plotfi added inline comments.
clang/lib/Driver/ToolChains/InterfaceStubs.cpp
15 ↗(On Diff #219226)

I could do that, but it would have to be:

namespace clang {
namespace driver {
namespace tools {
namespace ifstool {
...
} // namespace ifstool
} // namespace tools
} // namespace driver
} // namespace clang
plotfi updated this revision to Diff 221252.Sep 22 2019, 9:15 PM

Adding back -c. Fixing driver for .ifs files

plotfi marked an inline comment as done.Sep 22 2019, 9:20 PM
plotfi added inline comments.
clang/lib/Driver/Types.cpp
328 ↗(On Diff #214924)

I added -c back. Tried this and it works the same as without -emit-interface-stubs

$ build/bin/clang -c  -o - -emit-interface-stubs clang/test/InterfaceStubs/weak.cpp clang/test/InterfaceStubs/virtual.cpp 
clang-10: error: cannot specify -o when generating multiple output files
$ build/bin/clang -c  -o -  clang/test/InterfaceStubs/weak.cpp clang/test/InterfaceStubs/virtual.cpp 
clang-10: error: cannot specify -o when generating multiple output files
plotfi marked 3 inline comments as done.Sep 22 2019, 9:24 PM
plotfi updated this revision to Diff 221453.Sep 23 2019, 6:20 PM
plotfi updated this revision to Diff 221469.Sep 23 2019, 11:14 PM
compnerd added inline comments.Sep 24 2019, 8:02 AM
clang/lib/Driver/Driver.cpp
3372 ↗(On Diff #221469)

Does the interface merging have to be the last step? I could see interface merging preceding linking just fine.

3468 ↗(On Diff #221469)

Please update the unreachable message

clang/test/InterfaceStubs/driver-test.cpp
17 ↗(On Diff #221469)

Should we not ensure that no other symbols are exported?

clang/test/InterfaceStubs/merge-conflict-test.cpp
3 ↗(On Diff #221469)

Why not name the file merge-conflict-test.c?

4 ↗(On Diff #221469)

Why not leave the -target off entirely? That will allow you to drop the restriction of the x86-registered-target

clang/test/InterfaceStubs/object-double.cpp
2 ↗(On Diff #221469)

Similar

clang/test/InterfaceStubs/object-float.cpp
2 ↗(On Diff #221469)

And here

clang/test/InterfaceStubs/object.cpp
5 ↗(On Diff #221469)

Might as well as do it here as well

clang/test/InterfaceStubs/object.ifs
1 ↗(On Diff #221469)

Can we drop the target and get this to be portable?

clang/test/InterfaceStubs/template-namespace-function.cpp
2 ↗(On Diff #221469)

Try to drop the x86 requirement throughout

cishida added inline comments.Sep 24 2019, 8:02 AM
clang/include/clang/Driver/Options.td
635 ↗(On Diff #221469)

nit: Interface

clang/lib/Driver/ToolChains/InterfaceStubs.h
14 ↗(On Diff #221469)

is this used anywhere?

plotfi updated this revision to Diff 221851.Sep 25 2019, 3:44 PM
plotfi marked 6 inline comments as done.

addressing cindy and saleem's feedback

clang/lib/Driver/Driver.cpp
3372 ↗(On Diff #221469)

For now I think that's the expedient thing to do. Do you want to change that?

clang/test/InterfaceStubs/driver-test.cpp
17 ↗(On Diff #221469)

You mean like bar?

clang/test/InterfaceStubs/object.ifs
1 ↗(On Diff #221469)

Hmm, can we do unknown-unknown-linux-gnu?

plotfi marked 4 inline comments as done.Sep 25 2019, 4:41 PM
plotfi added inline comments.
clang/test/InterfaceStubs/template-namespace-function.cpp
2 ↗(On Diff #221469)

I ran into a yaml bug with windows name mangling. Can I do this in a follow up commit?

compnerd accepted this revision.Oct 1 2019, 9:41 PM
compnerd added inline comments.
clang/lib/Driver/Driver.cpp
3372 ↗(On Diff #221469)

Add a TODO perhaps?

clang/lib/Driver/ToolChains/InterfaceStubs.h
14 ↗(On Diff #221469)

Please remove the using namespace directive in the header.

This revision is now accepted and ready to land.Oct 1 2019, 9:41 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2019, 3:48 PM
tamur added a subscriber: tamur.Oct 9 2019, 11:09 AM

It seems that with this patch, llvm-ifs starts to depend on yaml2obj, which as far as I know, was only used for testing purposes until now. Is this intended?

plotfi marked 3 inline comments as done.EditedOct 9 2019, 11:30 AM

It seems that with this patch, llvm-ifs starts to depend on yaml2obj, which as far as I know, was only used for testing purposes until now. Is this intended?

No not with this patch, but with an earlier patch llvm-ifs does use yaml to generate elf. The library was available and elfabi wasn’t. I can move llvm-ifs to elfabi when it is ready. Hmm, on second thought this patch does pull in llvm-ifs so I take your point. This is intended, for now.

clang/lib/Driver/Driver.cpp
3372 ↗(On Diff #221469)

I agree with that.

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:
...
plotfi marked an inline comment as done.EditedOct 11 2019, 1:18 PM

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.

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?

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

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.

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??

I think we should not rely too much on the driver emitting object files. There can be many subtle differences (D68897).

When the -emit-obj output is feed to llvm-nm, the clang cc1 command line should have an explicit target triple.

clang -cc1 -triple x86_64-pc-windows /dev/null -emit-obj -o a.o

0000000000000000 b .bss
0000000000000000 d .data
0000000000000000 a @feat.00
0000000000000000 t .text

clang -cc1 -triple x86_64 /dev/null -emit-obj -o a.o
nm has no output.

clang -cc1 -triple x86_64-macos /dev/null -emit-obj -o a.o
GNU nm's macho port prints "no symbols"