This is an archive of the discontinued LLVM Phabricator instance.

[gold] Remove an external dependency to GNU binutils' header file
Needs RevisionPublic

Authored by ruiu on May 14 2022, 11:06 PM.

Details

Reviewers
MaskRay
tstellar
Summary

This patch adds a header file that defines the same data types and function declarations as the binutils' "plugin-api.h" so that we can build the gold LTO plugin without binutils source code.

Since we no longer directly include a binutil's file, the documentation section that says the reuslting LLVMgold.so has to be licensed under GPLv3 has been removed.

Diff Detail

Event Timeline

ruiu created this revision.May 14 2022, 11:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2022, 11:06 PM
ruiu requested review of this revision.May 14 2022, 11:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2022, 11:06 PM
ruiu set the repository for this revision to rG LLVM Github Monorepo.May 14 2022, 11:15 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 14 2022, 11:15 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript
MaskRay accepted this revision.May 16 2022, 12:38 PM
This revision is now accepted and ready to land.May 16 2022, 12:38 PM

I haven't checked but for ninja check-llvm-tools-gold, we still hope that the tests are disabled if the system does not provide gold. There may need a toggle for the tests.

pcc added a subscriber: pcc.May 16 2022, 12:43 PM
pcc added inline comments.
llvm/tools/gold/gold-plugin.cpp
44–52

Can we remove this stuff now?

ruiu added inline comments.Jun 1 2022, 5:59 PM
llvm/tools/gold/gold-plugin.cpp
44–52

I believe so, but I'll do that in another patch.

MaskRay added a comment.EditedJun 1 2022, 6:05 PM

It's better to move LLVM_BINUTILS_INCDIR removal for test directories into another patch. This change may expose some failures.

ruiu added a comment.Jun 1 2022, 6:18 PM

Does it? What is your test command?

ruiu added a comment.Jun 1 2022, 6:36 PM

I'm not sure if my credential is still valid. Do you mind if I ask you to submit this for me?

tstellar requested changes to this revision.Jun 1 2022, 6:37 PM
tstellar added a subscriber: tstellar.

What is the motivation for this change? I really don't like bundling external headers like this, because it can lead to subtle hard to catch bugs. It also makes it harder to distribute LLVM on operating systems with different versions of binutils.

Even if this change is accepted, we would need to have an attorney review whether or not this changes the license for LLVMgold.so before it can be committed.

This revision now requires changes to proceed.Jun 1 2022, 6:37 PM
ruiu added a comment.Jun 1 2022, 7:18 PM

The motivation of doing this is to be able to build LLVMgold.so without binutils' source files and make it clear that LLVMgold.so does not include any GPL code.

The header file defines the public interface between linker plugins and compilers (and other tools such as ar which has to read symbol table of LTO object files). New structs or constants may be added to this header, but the existing ones will never be deleted or altered in such a way that that breaks compatibility. So, the declarations in this file aren't different from other structs and types that are defined the same as they are in GNU systems. We already have lots of such structs and types in llvm/include, no?

The motivation of doing this is to be able to build LLVMgold.so without binutils' source files and make it clear that LLVMgold.so does not include any GPL code.

OK, as I mentioned. I think we need an attorney to review this change and confirm that it actually accomplishes this goal.

The header file defines the public interface between linker plugins and compilers (and other tools such as ar which has to read symbol table of LTO object files). New structs or constants may be added to this header, but the existing ones will never be deleted or altered in such a way that that breaks compatibility. So, the declarations in this file aren't different from other structs and types that are defined the same as they are in GNU systems. We already have lots of such structs and types in llvm/include, no?

So what happens if LLVMgold.so uses one of the new structs or constants and then is built and run on system where binutils is old enough to not have these new structs and constants?

ruiu added a comment.Jun 1 2022, 7:38 PM

So what happens if LLVMgold.so uses one of the new structs or constants and then is built and run on system where binutils is old enough to not have these new structs and constants?

The same thing can happen with GCC and binutils. Imagine that you install a newer version of GCC binary to your system and use it with an older version of binutils ld which doesn't know anything about new structs and constants. The plugin should still work, as changes to this file must be made in such a way that that don't break ABI. The whole point of this interface is decoupling compiler versions from linker versions. So it's backward compatible.

khchen added a subscriber: khchen.Jun 1 2022, 7:54 PM

IMO, maybe we could keep the DLLVM_BINUTILS_INCDIR option support but default is using the Plugin.h?

ruiu added a comment.Jun 1 2022, 7:58 PM

IMO, maybe we could keep the DLLVM_BINUTILS_INCDIR option support but default is using the Plugin.h?

We can, but I wonder what is the motivation of doing it.

ruiu added a comment.Jun 1 2022, 10:23 PM

OK, as I mentioned. I think we need an attorney to review this change and confirm that it actually accomplishes this goal.

Can you add an attorney as a reviewer of this change so that we can proceed?

tstellar added a subscriber: tonic.Jun 2 2022, 2:18 AM

OK, as I mentioned. I think we need an attorney to review this change and confirm that it actually accomplishes this goal.

Can you add an attorney as a reviewer of this change so that we can proceed?

Yes, I've notified @tonic and she will reach out to the attorney.

ruiu added a comment.Jun 27 2022, 11:30 PM

OK, as I mentioned. I think we need an attorney to review this change and confirm that it actually accomplishes this goal.

Can you add an attorney as a reviewer of this change so that we can proceed?

Yes, I've notified @tonic and she will reach out to the attorney.

@tstellar @tonic Did you guys get any response from the attorney?

OK, as I mentioned. I think we need an attorney to review this change and confirm that it actually accomplishes this goal.

Can you add an attorney as a reviewer of this change so that we can proceed?

Yes, I've notified @tonic and she will reach out to the attorney.

@tstellar @tonic Did you guys get any response from the attorney?

Yes, the header file will still be GPLv3 licensed even if it's copied into the LLVM tree, so even if we were to do this change, LLVMgold would still be GPLv3.

owl4ce added a subscriber: owl4ce.Aug 25 2022, 8:32 PM