In D107799#3598787, @thakis wrote:

https://github.com/llvm/llvm-project/commit/716e27bc9ad4699bbc82318834e938bcc9682cf8 turned this on for arm linux systems. Having this on by default in some configs on linux and off on others seems like it'd be very confusing. Let's please either have it on or off uniformly per OS?

I don't know how this works for you. CMake gets a list of implicit link dirs from parsing the ld command that clang -v outputs when linking. When $clang/lib/x86_64-unknown-linux-gnu exists, that's part of that output. Then, CMake determines the arch it later uses when finding libraries by matching architectures in each of the implicit link dirs, and $clang/lib/x86_64-unknown-linux-gnu is the first match. From there [it uses x86_64-unknown-linux-gnu](https://github.com/Kitware/CMake/blob/master/Source/cmSearchPath.cxx#L180), and of course it doesn't find libraries in /usr/lib/x86_64-linux-gnu because it never looks there in find_library. That can even affect pkg-config, because it uses that same derived architecture to find pkgconfig directories. When $clang/lib/x86_64-unknown-linux-gnu doesn't exist, /lib/x86_64-linux-gnu is the first match, and it uses x86_64-linux-gnu as arch, and that works.

Do you mean it found zlib?

Passing -Wl,/fixed as a linker flag when building the NSIS stubs fixes the issue, FWIW.

So, looking a little bit deeper, the resource editor in NSIS doesn't handle relocations, and the .reloc section is right after the .rsrc section that the resource editor will grow... So they actually have a build rule to pass /FIXED to link.exe when building their stubs with MSVC. I suppose mingw-binutils defaults to non-relocatable .exes (because the problem doesn't appear when building with mingw-gcc/mingw-binutils), and we only end up with relocatable .exes because we're building with mingw-clang/lld and lld defaults to relocatable .exes.
I'd call this a build system bug.

The base relocation table usually resides in the .reloc section, and as the exact size (0xB68) matches, it looks like it just has got the wrong RVA in the header. With the new, more thorough checks, libObject notices that the RVA points into the .ndata section (within 0x40000 - 0x5C000) but there's only raw data covering 0x40000 - 0x40200, and 0x43000 is outside of that. Previously we'd just set up a pointer into bogus/out of range data in these cases, now we error out.

Minimal reproducer:

• git checkout 311f7839602344ca347816146edb68c0ffaaa060
• cmake llvm -B obj -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/llvm/stage1 -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_ENABLE_RUNTIMES="compiler-rt;libcxx;libcxxabi"
• ninja -C obj install
• cmake llvm -B obj2 -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang" -DCMAKE_C_COMPILER=/tmp/llvm/stage1/bin/clang

In D126898#3565873, @mstorsjo wrote:

Can you upload that binary somewhere?

I don't have time to provide more details right now, but this change broke Firefox mingw builds with: llvm-strip: error: '../../dist/firefox/uninstall/helper.exe': RVA 0x43000 for base reloc table found but data is incomplete

But it is irrelevant here

What is your $HOME/Stable/bin/clang? it has to be a clang with the patch applied.

IOW, nothing changed since https://reviews.llvm.org/D107799#3027607

I suspect even eliminating the machine part of the target, there could still be problems with i386 vs i686.

This is causing problems finding system zlib when compiling clang with clang, because when using the target directories for runtimes, cmake determines the target name to match that, and then proceeds to try to find zlib.h in /usr/include/$target, which doesn't work when $target doesn't match the system multi-arch triple, which it doesn't on Debian systems because the runtime directory and the cmake target is e.g. x86_64-unknown-linux-gnu, and the system multiarch triple is x86_64-linux-gnu...

In D118214#3446233, @ab wrote:

D'oh, this fell through the cracks, sorry folks! I committed the fix I had in mind (cfa4fe7c5187).

FWIW, We're seeing Firefox crashes on aarch64 macos that have been bisected to this change as well. Top frames look like:

For some reason this change makes Firefox Linux ASAN builds break our CI in a way that doesn't even leave logs (it looks like the workers completely die)

It turns out CMAKE_*_FLAGS is only initialized from CMAKE_*_FLAGS_INIT _after_ CMAKE_DETERMINE_COMPILER_ID is done.

It still happens when I cut down to CMAKE_TOOLCHAIN_FILE, LLVM_NATIVE_TOOLCHAIN, MSVC_BASE, WINSDK_BASE, WINSDK_VER and HOST_ARCH.

The only variables I'm passing: CMAKE_C_COMPILER_TARGET, CMAKE_CXX_COMPILER_TARGET, CMAKE_ASM_COMPILER_TARGET, CMAKE_BUILD_TYPE, LLVM_ENABLE_ASSERTIONS, LLVM_CONFIG_PATH, COMPILER_RT_DEFAULT_TARGET_ONLY, CMAKE_TOOLCHAIN_FILE, LLVM_NATIVE_TOOLCHAIN, MSVC_BASE, WINSDK_BASE, WINSDK_VER, HOST_ARCH.

Somehow this change broke cross compiling for 32-bit windows using WinMsvc.cmake. Cmake ends up adding /machine:x64 to the linker flags during one of its first checks, and that fails.

This doesn't leave much room to use __attribute__((format(printf))) on custom printf implementations that do support %n on Android does it?

@int3 Can you commit this for me?

Addressed comments and added a check that __thread_vars is aligned correctly for the test to be valuable.

Applied clang-format.

FYI, I filed https://github.com/llvm/llvm-project/issues/52681 for a crash that this patch causes.

Reduced testcase:

enum FrameIID {
  nsBox_id,
  nsIFrame_id,
  nsHTMLFramesetBlankFrame_id,
  nsHTMLFramesetBorderFrame_id,
};

So far, what I've found is that in some cases, DIEnumerator::get returns the same node for similar enumerator values in different enums that happen to have the same name and value (even if their bitwidth differs), but sometimes not.
For example, in one compilation I see:

DIEnumerator::get (Name={Data@0x5575ee17ea38="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=8}, Context@0x5575e8305da0.pImpl=0x5575e8307230) = 0x55760563a708
DIEnumerator::get (Name={Data@0x5575ee17ea38="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, Context@0x5575e8305da0.pImpl=0x5575e8307230) = 0x5576067e7148

In another compilation of the same file with the same flags:

DIEnumerator::get (Name={Data@0x561c659e0cc8="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=8}, Context@0x561c5fb68da0.pImpl=0x561c5fb6a230) = 0x561c7ce9dbf8
DIEnumerator::get (Name={Data@0x561c659e0cc8="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, Context@0x561c5fb68da0.pImpl=0x561c5fb6a230) = 0x561c7ce9dbf8

Not that it matters anymore after D111123 but please note that trunk as of the landing of this (or even trunk before the landing of D111123) says that raw profiles emitted by clang 13.0.0 (which has a backport of this) are malformed.

In D106585#3125280, @rnk wrote:

In D106585#3122726, @glandium wrote:

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

Are you sure you've arrived at the right code review? This change, D106585, modified type information, which is unrelated to the change you linked to. We have also noticed *other* determinism issues (follow up with @hans to learn more), but they aren't related to this change, which is from July.

In D106585#3122726, @glandium wrote:

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

In D91722#3121293, @StephenTozer wrote:

In D91722#3101440, @glandium wrote:

The relanding in 47633af9d4a8b93f50cb711cf23489736e0226f1 broke determinism for Firefox.

Has the non-determinism problem been fixed with the landing of rG3c47c5ca13b8a502de3272e8105548715947b7a8?

This broke determinism when building Firefox.

The relanding in 47633af9d4a8b93f50cb711cf23489736e0226f1 broke determinism for Firefox.

In D110224#3015751, @thakis wrote:

lg.

out of interest, which SDK version has this problem?

LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR was removed in https://reviews.llvm.org/D58157, so it's unset by default.

Firefox CI is using a custom clang, but uses a NDK otherwise (an old-ish one, clearly older than r22 which is the first that defaults to lld). And we do pass -fuse-ld=bfd at the moment for $reasons.
If clang _really_ wants to assume lld as the linker for android, then it should make using -fuse-ld=somethingelse an error and invoke ld.lld rather than ld, if it doesn't already do that.

"Android only supports lld" might be true now, but it hasn't always been true, which means the change is not backwards compatible with versions of the NDK that don't use lld. Also, -fuse-ld is still a valid flag that can allow to use a different linker than lld.

FWIW, when applying this patch on a LLVM10-based rustc, the rustc build fails with:

PHI node has multiple entries for the same basic block with different incoming values!
  %35 = phi { i64*, i32 }* [ %83, %77 ], [ %84, %77 ], [ %33, %2 ]
label %77
  %83 = getelementptr { i64*, i32 }, { i64*, i32 }* %35, i64 -1
  %84 = getelementptr { i64*, i32 }, { i64*, i32 }* %35, i64 -1
in function _ZN80_$LT$alloc..vec..Vec$LT$T$GT$$u20$as$u20$alloc..vec..SpecExtend$LT$T$C$I$GT$$GT$9from_iter17h62b0d0798a862a64E
LLVM ERROR: Broken function found, compilation aborted!
error: could not compile `rustc_typeck`.

In D57636#1389971, @efriedma wrote:

LGTM. Do you want me to commit this for you?

@efriedma can you take another look? Ideally, this should be backported to the release_80 branch, so that would need to be landed asap.

Updated EmitAArch64BuiltinExpr per https://reviews.llvm.org/D57636#1383751 and the testcase per https://reviews.llvm.org/D57636#1384348

In D57636#1383566, @efriedma wrote:

(It should be possible to check that we aren't inserting incorrect truncation/extension operations in the IR.)

Opened D57636

In D51204#1271564, @dmajor wrote:

In D51204#1253230, @mgrang wrote:

Will abandon this patch since I have implementations of these which I will upstream soon.

Just to link up the reviews: these landed in D52838 and D53115. (Thanks @mgrang!)

In D54891#1311086, @olista01 wrote:

I can provide a full log of the 4831 warnings marked -Winline-asm during a Firefox for ARM Android build, if you're interested.

Yes, it would be helpful if you could send me that output (with the latest patch applied).

Correction, the mpi_arm.c ones seem to be caught. However, warnings are also being emitted for assembly post substitution. Example:

Assuming removing the variadicOpsAreDefs test completely (since I couldn't figure where it came from) still yields something that mostly works, it appears not to catch when the assembly writes to input operands. Examples of real code that had the problem:

• https://hg.mozilla.org/projects/nss/rev/09ab3310e71028dbdbf6846289cb4d3cea249534
• https://hg.mozilla.org/mozilla-central/rev/e471c532fd11

Updated wording.