LGTM as well.

In D40660#2686669, @mstorsjo wrote:

I see that @smeenai brought up the inconsistency between _DLL and _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS already back when this was reviewed. I'm running into problems with this inconsistency in various cases.

_DLL is tied to the choice of CRT (compiling with -MD defines it, compiling with -MT doesn't define it), while we might have built and be using either a statically linked or dynamically linked libc++. And as @smeenai argued, if _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS is not defined, then all declarations are decorated with dllimport, and there's no doubt about it, we have to use the DLL form of the library.

This is noticable today when running the libc++ testsuite (which is hooked up in CI these days, so by posting a patch you can get it verified too!); currently the CI configuration builds with both shared and static enabled (which links and uses the shared library only). If disabling building the static library, however, testing fails, as the autolinking tries to pull in the static library.

(We could disable autolinking within the test suite altogether, as it explicitly links the right library anyway, but I'd rather fix the autolinking feature instead of having to disable it because it misbehaves.)

I guess it's possible to set up cases with _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS disabled but still linking against the DLL, that might work at least for some subsets of the library functionality. But if manually enabling _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS in a DLL configuration, then one can also define _LIBCPP_NO_AUTO_LINK at the same time to opt out of the autolinking.

Therefore, I would like to post a patch that fixes the autolinking mechanism to rely on _LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS which is closely tied to whether dllimport is used, and to whether a shared or static library was built. (And if both were built, it defaults to the shared one.)

WDYT @smeenai @compnerd?

Unless you think it only makes sense when paired with the rest of this diff, could you split out the renaming of SubsectionMapping to SubsectionMap into its own NFC diff (and probably just commit that directly)? It'd make the rest of this diff easier to read.

Good catch.

In D97007#2636289, @int3 wrote:

2. We could make MachHeaderSection inherit from an InputSection rather than a OutputSection (SyntheticSection), and then have it be part of the __TEXT MergedOutputSection. This is more architecturally consistent, but would probably require a good deal of refactoring.

In D93203#2647578, @bd1976llvm wrote:

@compnerd, @smeenai do you have any further comments?

In D99217#2646288, @smeenai wrote:

In D99217#2646114, @thakis wrote:

This seems to break the test on linux: http://45.33.8.238/linux/42505/step_11.txt

Probably because this now needs two spaces between : and LLD

Oops, I missed your comment before making mine. I thought not having strict whitespace would take care of this, but maybe not?

In D99217#2646114, @thakis wrote:

This seems to break the test on linux: http://45.33.8.238/linux/42505/step_11.txt

Probably because this now needs two spaces between : and LLD

Got it, thanks for all the info!

In D96403#2638932, @rprichard wrote:

In D96403#2638883, @smeenai wrote:

With NDK r22, I only see libunwind.a for armv7. Will it be provided for other architectures in future NDK versions?

NDK r23 should have a libunwind.a for all architectures. This change isn't in the r23 beta 2 that was just released, but it is in the current NDK canary build at https://ci.android.com/builds/branches/aosp-master-ndk/grid. (e.g. build 7220482).

With NDK r22, I only see libunwind.a for armv7. Will it be provided for other architectures in future NDK versions?

Sounds good. I'll let Fangrui take a look then :)

The summary says !isLocal but the code says !isLazy. I'm assuming the code is correct.

LGTM

This will already work as-is for the runtimes build (since it builds the builtins standalone), but builtins-config-ix already sets an equivalent option for compiler-rt's custom macros, so it should be fine to apply this one universally to the builtins as well (either in this file or in builtins-config-ix).

Hilariously enough, this breaks building compiler-rt itself inside LLVM's runtime builds setup for us. The runtimes build setup builds clang and then uses the just-built clang to build compiler-rt. That build fails to link since my just-built clang doesn't have compiler-rt available, because it's currently trying to build compiler-rt itself. That's a bug in the compiler-rt build system, and I sent out https://lists.llvm.org/pipermail/llvm-dev/2021-March/149137.html to ask what we should do about it.

In D96164#2605092, @thakis wrote:

Did this land without any tests?

LGTM

In D97878#2602077, @plotfi wrote:

This makes sense to me. I approve. Can we move the 3/60 seconds number to a const int value set somewhere higher up in the file as a global with a comment explaining this as well?

Address review comments

LGTM

Sorry for the delay here. This looks good; thank you!

In D97314#2588702, @phosek wrote:

In D97314#2588445, @ldionne wrote:

In D97314#2585569, @phosek wrote:

I'm a bit worried about using TARGET cxx-headers. Whether that evaluates to TRUE or FALSE depends on the order of target evaluation, which in this case is determined by the order in which you put projects into LLVM_ENABLE_PROJECTS/LLVM_ENABLE_RUNTIMES. I ran into this recently in D97256. Ideally we would use generator expressions but they don't support dependencies. Alternative is to either use patterns like "libcxx" IN_LIST LLVM_ENABLE_RUNTIMES or canonicalizing the order in which we process targets (which we already do in some cases). @smeenai, @beanz, @compnerd do you have any thoughts on this/other ideas?

Could we create those targets upfront regardless of the order projects are included in? And then we'd populate those targets later on in their respective CMake files, but at least they would be visible globally. Kind of like introducing a forward-declaration in C++ to break circular dependencies.

Would that make sense?

That's an interesting idea, I haven't thought of that but I like it. The question is where would those targets get defined. Could we use the top-level runtimes build for that purpose?

In D92808#2593331, @ahatanak wrote:

In D92808#2593204, @thakis wrote:

FYI, we're seeing test failures caused by this patch in iOS/arm64 builds when arc is used (simulator is fine though): https://bugs.chromium.org/p/chromium/issues/detail?id=1182642#c11

We'll try to investigate a bit more on our side, but I wanted to give an early(ish) heads-up in case others see this or whatnot.

Not sure if this landed before or after the 12.0 branch.

Thank you. Let me know when you have more information.

This isn't in release/12.x. The older version of the patch, which used attributes instead of operand bundles, was in release/12.x, but got reverted later.

I suppose it's more efficient to have a single iteration of the argument list, but I also don't think argument parsing efficiency matters at all (or at least doesn't matter enough for this change to be a problem), so LGTM.

If you care about eventually getting rid of the old check-clang-tools alias, you'd want to have a mailing list announcement/deprecation notices/etc. (I don't think leaving it around does any harm though.)

Is this rebased on main? The LLD for Mach-O changes look like they belong to a different diff.

In D97610#2595963, @thakis wrote:

b) What's the motivation for this change? lld assumes that inputs are produced by a compiler or similar, and are generally valid. What problem is this solving?

LGTM

LGTM

LGTM

In D96613#2590948, @MaskRay wrote:

• If you want to integrate the functionality in your build system (e.g. you're trying to generate an aggregated report for all the libraries you build), a pipeline is much harder to integrate into your build system, vs. just adding an argument.
• You have to worry about platform differences; e.g., lots of Linux utilities have different arguments or behaviors than their BSD (and therefore macOS) equivalents, and Windows doesn't have these utilities at all.

I think a small set of composable options for build dependency analysis will be useful, but we need to consolidate the requests and think of their composability/maintenance/etc. Apologies but I feel that "Linux utilities have different arguments or behaviors than their BSD" in this particular context is probably a weak argument: here we use LLVM binary utilities and a common 'awk' (which can be reimplemented in build system language straightforwardly). Many downstream groups have bundled LLVM binary utilities as platform-neutral utilities which are already used heavily in build systems. We don't necessarily re-invent features provided by them in LLD.

In D96613#2590948, @MaskRay wrote:

In D96613#2590895, @smeenai wrote:

In D96613#2590870, @MaskRay wrote:

https://sourceware.org/bugzilla/show_bug.cgi?id=27407
I think the functionality can be straightforwardly emulated. For example, llvm-nm -Du dumps undefined symbols in .dynsym.

% ld.lld @response.txt $(llvm-nm -Du usr/lib64/libc.so.6 | awk '{print "-y"substr($0,20)}')

This even works with versioned symbols while the patch doesn't. You can tweak -u: e.g. -U dumps defined symbols (-U is not in nm).
This is the flexibility provided by composing tools, which will be a bit inconvenient to implement in code.

The patch uses this->file, however, the value may change if the symbol gets resolved to other files. In the cases I can conceive we want the trace of the full lifetime of a symbol, not only when it is bound to a specific file.

In general, while there's a lot of value in being able to compose tools, I strongly disagree that it's an adequate replacement for having functionality directly built into a tool:

My opinion on this is still case by case. For the particular features, (1) the lack of customization of -u/-U` and (2) the this->file usage is my main concern about the new option.
(And a minor concern: needToTraceSymbol cost - that was why I re-stated that whether it would regress the symbol resolution performance)

In D96613#2590870, @MaskRay wrote:

https://sourceware.org/bugzilla/show_bug.cgi?id=27407
I think the functionality can be straightforwardly emulated. For example, llvm-nm -Du dumps undefined symbols in .dynsym.

% ld.lld @response.txt $(llvm-nm -Du usr/lib64/libc.so.6 | awk '{print "-y"substr($0,20)}')

This even works with versioned symbols while the patch doesn't. You can tweak -u: e.g. -U dumps defined symbols (-U is not in nm).
This is the flexibility provided by composing tools, which will be a bit inconvenient to implement in code.

The patch uses this->file, however, the value may change if the symbol gets resolved to other files. In the cases I can conceive we want the trace of the full lifetime of a symbol, not only when it is bound to a specific file.

LGTM

LGTM. Do you have commit access?

My understanding of the issue is that, with the runtimes build, the cxx target will get created after we process LLDB's build files. The TARGET cxx check here will therefore fail, but the cxx target will end up existing later, and CMake is perfectly fine adding a dependency to a target that gets created later, so the failing check is spurious. The libcxx IN_LIST LLVM_ENABLE_RUNTIMES is basically a proxy for "the cxx target doesn't exist right now but will exist later". Could you add an explanation of the issue to the summary or the code comment?

Will this fix https://bugs.llvm.org/show_bug.cgi?id=17550?

Adding some reviewers who've contributed to the SHA1 implementation.

The NATIVE build is also used for cross-compilation scenarios (e.g. you're building on Windows but targeting Linux, but you still need tools like tablegen to be built targeting Windows), and for those cases, using the same CMAKE_TOOLCHAIN_FILE as the main build is completely incorrect.

It works! Thanks @mehdi_amini

Take 2

LGTM

In D96164#2547390, @thakis wrote:

In D96164#2547158, @gkm wrote:

In order to perform ad-hoc codesign, I will need to compute sha256 of the program's pages. I see no other part of LLVM that uses a sha256 library. What library should I use?

LLVM tries to not depend on external libraries if possible. Given that sha256 isn't that much code (https://golang.org/src/crypto/sha256/), just adding a fresh implementation of it (to lld/MachO for now) is probably the way to go.

A few other cases that might be good to have test cases for as well, but I'll let you judge that:

• Empty (and non-empty) archive members
• Empty (and non-empty) universal binary members, possibly with and without -arch_only

In D95843#2540709, @alexshap wrote:

@smeenai : many thanks for the review,

1. regarding bitcode files: cctools libtool doesn't warn on them
2. regarding input files which are filtered out by -arch_only: cctools libtool doesn't warn on them either

Perhaps, it makes sense to add tests for these cases

Should we warn on files that would have been filtered out by -arch_only? (What does cctools libtool do?)

LGTM

LGTM

I saw this on LLVM Weekly, and I like it a lot :)

I'm also confused how the ALWAYS_GENERATE name related to its actual functionality ... might have been something historic?

LGTM

We've been pretty liberal with our use of option groups, mostly to organize the help text. I agree that the commit message should reflect that there's some new version options being added as well (tvos, watchos, bridgeos, and driverkit, if I'm reading the diff right).

Running the unit test with ASAN gives me the following. The stack trace isn't the most helpful, unfortunately, but it should hopefully be something to go on.

It's been a long time since I've contributed to libc++, and the pre-merge CI setup is a massive improvement over what we had before. A huge kudos to everyone who made it possible :)

Rebase

Thanks, I committed this. You might get some buildbot failure emails. Given the nature of this change, they're overwhelmingly likely to be false positives, but feel free to forward them to me if you want confirmation. (Search for me in LLVM's git log to get my email.)