@ldionne PTAL

OS_NAME MATCHES "Darwin|Linux|FreeBSD|Android|NetBSD" AND ANDROID_PLATFORM_LEVEL GREATER 23

LGTM once the comments are addressed. (I'd also approve the alternative approach discussed in the thread of searching each older API level directory, but have no preference one way or the other)

Doesn't seem like we use this for Android, but @pirama just in case.

Nice, that's a lot of code cleaned up! LGTM, but probably should wait for someone from Apple to weigh in. I think the new formatting for those error messages is better for them too but that's not my call :)

Fine by me if this is the default for LLD.

Fixes LGTM.

Odd. Thanks for confirming. We'll revert while we try to figure out what it is in that builders config that is uncovering the bug. I don't think the default config uses libc++, so probably that?

Are you sure that's caused by this change? We didn't see that error.

LGTM. @vitalybuka, do you want to have one last look, or are we clear to merge?

LGTM but we should wait for @vitalybuka.

In D108394#3003237, @vitalybuka wrote:

I guess testing is getting more complicated.
Maybe we should separate tests and non-test patches here? I suspect we well have several of them. Even after lit will run them as expected, they will likely just crash on start.
WDYT?

I think this is probably fine from the NDK perspective. Even before Clang defaulted to C++11 or newer, our build system did that when using libc++. Other build systems may not have, but unfortunately I'm not currently able to get at the data to get a concrete answer.

(I'd still like to hear what vitalybuka/eugenis have to say, but LGTM)

A couple stray pieces snuck in. Could you revert those and update the change? It looks like you also missed the mailing list, but we can fix that once the patch is fixed.

In D102570#2914756, @MaskRay wrote:

In D102570#2914731, @smeenai wrote:

Do you plan to land this? I thought of it as potentially useful for https://github.com/android/ndk/issues/1551. The issue has the full details, but the summary is that we only want a single copy of __emutls_get_address that's used by all shared objects in a process, otherwise different DSOs will see different emulated TLS state for the same variable. One way to ensure that would be to put the emulated TLS bits in their own DSO and have everyone else reference that, but a full DSO is pretty heavy-weight. Another would be to make it default visibility and weak and let the dynamic linker coalesce multiple copies at runtime; -Bsymbolic and -Bsymbolic-functions would break that, but -Bsymbolic-non-weak-functions wouldn't.

I was waiting for users:) Glad that this can find a user.

LGTM, but @rprichard knows this area better than I do so he should probably weigh in before submitting :)

Fix remaining tests for the deferred test configuration.

Adds XFAIL-BUILD to deal with the deferred-run cases that fail to build.

Initial pass at my suggestion of deferring XFAIL handling to the bundler. This is incomplete because we'll need to differentiate XFAIL-BUILD and XFAIL-RUN in tests (running this as-is makes any test that is XFAIL because it doesn't _build_ turn to FAIL, but things that fail to _run_ are handled correctly), but it's enough to get feedback on the idea.

Re problems 1 and 2 (xfail handling), my idea what that we'd add --xfail to the bundler interface and delegate to the test runner to handle that. For Android I need to fit these tests into tradefed, so our bundler would work by creating the test config files for tradefed and would let it handle xfails. We don't have much reason to interact with LIT again after the tests are built. This more or less matches how we've been running libc++ tests for years, though with some improvements (xfail info is currently manually tracked rather than being given by LIT). I've been carrying a local patch that adds a --build-only flag which just ignores xfail annotations and defers all the execution and interpretation to our test runner.

In D91841#2540868, @rprichard wrote:

In D91841#2428875, @MaskRay wrote:

Implementing the relevant functions called by divxc3 is one choice, another choice is to emulate libgcc - don't use scalbn.

The libgcc version goes about the computation in a slightly different way -- e.g. it computes an intermediate ratio of c / d or d / c (ensuring that the fabs(ratio) is at most 1.0) to avoid overflow, whereas the current compiler-rt version uses scalbn (and closely matches the reference implementation in Annex G of the C specification). The scalbn approach seems to be more accurate? e.g.:

• From G.5.1 Multiplicative operators (paragraph 9):

Scaling the denominator alleviates the main overflow and underflow problem, which is more serious than for multiplication.
In the spirit of the multiplication example above, this code does not defend against overflow and underflow in the calculation
of the numerator. Scaling with the scalbn function, instead of with division, provides better roundoff characteristics.

• From the libgcc source:

/* ??? We can get better behavior from logarithmic scaling instead of
   the division.  But that would mean starting to link libgcc against
   libm.  We could implement something akin to ldexp/frexp as gcc builtins
   fairly easily...  */

• https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59714#c8

I'm guessing we wouldn't want to reduce accuracy to break the libm dependency?

It sounds like everyone is happy here but the tools. Could we get a libunwind reviewer (preferably @compnerd, since his review is the red one) to LGTM this?

In D95166#2540705, @thakis wrote:

In D95166#2540399, @danalbert wrote:

We've since added -fuse-ld=lld to the three CMAKE_*_FLAGS so I think we're likely set on our end.

https://reviews.llvm.org/D76452 not being accepted means that Android toolchains must have LLD installed as ld.

If you pass -fuse-ld=lld everwhere, that's not needed, right (?)

We've since added -fuse-ld=lld to the three CMAKE_*_FLAGS so I think we're likely set on our end.

In D95166#2530791, @thakis wrote:

Landed revert in 1608ba09462d877111230e9461b895f696f8fcb1. Someone should file a PR to make sure that gets merged to the 12.0 branch.

We can then reland on trunk once there are clear instructions for folks building with trunk clang but old NDK but without the toolchain file (either "use toolchain file" or "set LLVM_USE_LLD" or something like that, I'm guessing?)

In D95166#2529412, @dmajor wrote:

It does feel kind of awkward to me that _this_ is the patch that ends up breaking the builds, versus something at the cmake level that says "you are explicitly unsupported".

In D95166#2527437, @vitalybuka wrote:

Our Android build bot is broken after this patch http://lab.llvm.org:8011/#/builders/77/builds/3234

In D95166#2525131, @dmajor wrote:

Firefox has a build break from this change. In certain Android configurations we use bfd or gold. The statement in the commit message "Android only supports LLD" is news to me, could you point me to any references for this?

Thanks! Has been up for a while and I haven't heard any objections. Will submit and we can revert if issues are found.

https://github.com/llvm/llvm-project/commit/0849047860a343d8bcf1f828a82d585e89079943

Ping.

Ping

In D87950#2354838, @ldionne wrote:

Actually, talk about timing! The Clang patch was just posted here: https://reviews.llvm.org/D90188

LGTM for Android.

In D81622#2094409, @ldionne wrote:

While this doesn't look wrong to me -- and the correctness of this depends entirely on where vendors decide to put their headers so it's hard for me to verify -- I'm wondering why not all toolchains use this mechanism. We seem to be adding an abstraction that's used only by some toolchains, but not all. I think it would be great to have a single canonical way of representing the system root.

The NDK still supports linkers other than LLD, but we are changing the default to LLD in the next release. I'd prefer to keep this for the time being, but don't feel strongly about it.

In D76452#2002981, @danalbert wrote:

In D76452#2002917, @MaskRay wrote:

In D76452#2002875, @danalbert wrote:

In D76452#2002856, @int3 wrote:

Yes, I was referring to that question too :) I'm working on the new lld-macho implementation, under the DarwinNew flavor. I'm not sure if anything depends on the old Darwin flavor, which is why we haven't removed it yet, though we plan to do that once we get the new implementation to a more mature stage.

Ah, gotcha :) Thanks! Will wait for @ruiu to chime in.

I vote for deleting the #ifdef __APPLE__ chunk so we don't have to add more code to either clang or lld....
The code owner of the existing lld darwin has explicitly expressed that we can drop the existing Darwin flavor at any time.

SGTM. Abandoning this. I'll send a patch to remove the LLD side shortly.

In D76452#2002917, @MaskRay wrote:

In D76452#2002875, @danalbert wrote:

In D76452#2002856, @int3 wrote:

Yes, I was referring to that question too :) I'm working on the new lld-macho implementation, under the DarwinNew flavor. I'm not sure if anything depends on the old Darwin flavor, which is why we haven't removed it yet, though we plan to do that once we get the new implementation to a more mature stage.

Ah, gotcha :) Thanks! Will wait for @ruiu to chime in.

I vote for deleting the #ifdef __APPLE__ chunk so we don't have to add more code to either clang or lld....
The code owner of the existing lld darwin has explicitly expressed that we can drop the existing Darwin flavor at any time.

In D76452#2002856, @int3 wrote:

Yes, I was referring to that question too :) I'm working on the new lld-macho implementation, under the DarwinNew flavor. I'm not sure if anything depends on the old Darwin flavor, which is why we haven't removed it yet, though we plan to do that once we get the new implementation to a more mature stage.

In D76452#2002620, @int3 wrote:

I don't think I have enough context here to answer the question, but I'm pretty sure that change wouldn't affect what I'm working on

In D76452#2000812, @nickdesaulniers wrote:

Can we use -DCLANG_DEFAULT_LINKER=lld to configure AOSP's distribution of LLD, then require the use of -fuse-ld=<whatever the host linker is on OSX that is currently used> when targeting OSX host tools?

@srhines to confirm that this won't break our toolchain build, and @rprichard because he's the one looking more closely at libunwind these days.

Option 4 was (at least on the surface) super easy: https://reviews.llvm.org/D78328. lmk if you'd prefer that approach. I'm slightly less confident in it since it affects non-Android platforms as well.

@MaskRay Any other ideas, or should I submit this? Reviewing all our options:

Added missing copyright header and moved into the libcxx specific test directory.

In D76452#1945084, @srhines wrote:

In D76452#1945029, @MaskRay wrote:

To cross build ELF object on macOS, another alternative is a wrapper named ld which invokes lld -flavor gnu "$@"

@danalbert Would this kind of idea work with your other reverted patch? I'm not sure exactly what broke on those builds.

It seems I'd goofed something in my testing earlier (I think I still had -fuse-ld=lld force on in my build system). While Clang will find ld in the driver directory and prefer it, LLD defaults to the Darwin driver mode when argv[0] is ld when run on Darwin. We need GNU mode, and the best way to get this behavior is to have Clang invoke ld.lld instead.