This is an archive of the discontinued LLVM Phabricator instance.

lld: Bundle compact_unwind_encoding.h in lld to fix stand-alone builds
AbandonedPublic

Authored by tstellar on Apr 22 2022, 6:00 PM.

Details

Reviewers
smeenai
Group Reviewers
Restricted Project
Summary

The monorepo build will still use the include from libunwind/ so nothing
has changed for that configuration. There is also now a check to ensure
that the file in lld/ and the file in libunwind/ are in sync.

Diff Detail

Event Timeline

tstellar created this revision.Apr 22 2022, 6:00 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 22 2022, 6:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
tstellar requested review of this revision.Apr 22 2022, 6:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2022, 6:00 PM

To be clear, this is to support a build scenario where you only have the source tree under lld available?

lld/MachO/CMakeLists.txt
17

Is this a debugging leftover?

tstellar updated this revision to Diff 424676.Apr 22 2022, 6:28 PM

Remove debug message.

tstellar marked an inline comment as done.Apr 22 2022, 6:28 PM

To be clear, this is to support a build scenario where you only have the source tree under lld available?

Yes, that's correct.

To be clear, this is to support a build scenario where you only have the source tree under lld available?

Yes, that's correct.

Ah, I'd missed that part of the definition in https://discourse.llvm.org/t/rfc-stand-alone-build-support/61291. Thanks for confirming.

The duplication here is really unfortunate, especially cos the burden of keeping the copied header up-to-date will fall on non-standalone build users. On the other hand, this header hasn't seen actual (non-license) update since 2016, so I imagine it'll remain pretty stable going forward.

An alternative would have been to move the header into LLVM (I think BinaryFormat would be a reasonable place for it), and then have libunwind and LLD both use it from there. It's still unclear if libunwind can use LLVM headers though; I don't see any existing usage, and the demangler is still duplicated between libc++abi and LLVM, for example.

All things considered, I'm fine with this; I'll give others some time to chime in as well though.

To be clear, this is to support a build scenario where you only have the source tree under lld available?

Yes, that's correct.

Ah, I'd missed that part of the definition in https://discourse.llvm.org/t/rfc-stand-alone-build-support/61291. Thanks for confirming.

The duplication here is really unfortunate, especially cos the burden of keeping the copied header up-to-date will fall on non-standalone build users. On the other hand, this header hasn't seen actual (non-license) update since 2016, so I imagine it'll remain pretty stable going forward.

Yes, I tried to ease this burden a little bit by failing the build if the bundled file was different than the version in libunwind, so at least buildbots would catch it and notify whoever modified the file in libunwind.

int3 added a subscriber: int3.Apr 23 2022, 12:09 PM

Q: when will we have a situation where the contents of the monorepo aren't fully available? I get that standalone builds exist to avoid having to build the entire repo, but I was under the impression that they would still be done using a full checkout of llvm-project... is that not right?

Q: when will we have a situation where the contents of the monorepo aren't fully available? I get that standalone builds exist to avoid having to build the entire repo, but I was under the impression that they would still be done using a full checkout of llvm-project... is that not right?

I am similarly confused. I wish the summary provides some the cmake command line.

thakis added a subscriber: thakis.Apr 23 2022, 2:08 PM

If standalone builds are something we want to support (not clear to me), we need to find some way to do them that doesn't require changes like this. This imposes a huge cost on everyone for sometime we haven't needed for years.

For the record, we're using standalone builds on Gentoo and I don't think we need anything like this — we have to fetch the complete llvmorg tarball anyway, so we have no problem unpacking the header files from libunwind.

@int3 @MaskRay The kind of stand-alone builds we are trying to support are documented here: D123968

There is more discussion on the rationale for supporting stand-alone builds here:
https://discourse.llvm.org/t/rfc-stand-alone-build-support/61291

int3 added a comment.Apr 25 2022, 11:28 AM

I see, so you're trying to support the sparse checkout case. How important is that? It doesn't seem like the LLVM repo is so big as to make sparse checkouts that much of a perf win...

smeenai requested changes to this revision.Apr 28 2022, 1:40 PM
smeenai added subscribers: phosek, ldionne.

While we're figuring this out ... @ldionne, @phosek, do the runtimes build changes (https://libcxx.llvm.org/BuildingLibcxx.html) also mean that we assume a full LLVM checkout is present? If so, we could move this header to LLVM and have both libunwind and LLD use it from there, which would sidestep the issue here.

This revision now requires changes to proceed.Apr 28 2022, 1:40 PM

I think for now what I'll do for now is just update D124405 to list libunwind as a required sub-directory for lld stand-alone builds. This will get me unblocked and make it possible to set up the CI.

tstellar abandoned this revision.May 17 2022, 1:18 PM