The OpenJDK bug was UB in the OpenJDK code: https://bugs.openjdk.java.net/browse/JDK-8229258 already fixed in JDK14. (But not backported to JDK 11 LTS, which is the version Brooks found an error in above.)

It seems like there's a bug with vtable thunks getting the wrong information. This appears to be a pre-existing bug, but this change has caused it to start actively breaking code.

In D99790#2677917, @brooksmoses wrote:

As a heads up, I'm seeing segfaults on internal code as a result of this change, as well as errors in Eigen's unalignedassert.cpp test (specifically, this line asserts: https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151).

In general, I think it's extremely unfortunate that Clang and LLVM have different copies of the same information. It's a problem for way more than just this one situation. So, I really don't like choice 1 -- I think it's moving in the wrong direction.

In D97224#2621328, @rjmccall wrote:

In D97224#2604537, @jyknight wrote:

I'm less certain about what to do with the __atomic_* builtins

The __atomic builtins have already been supporting under-aligned pointers all along -- and that behavior is unchanged by this patch.

How so? Clang hasn't been passing down an alignment, which means that it's been building atomicrmw instructions with the natural alignment for the IR type, which means we've been assuming that all pointers have a least that alignment. The LLVM documentation even says that atomicrmw doesn't allow under-alignment.

Use natural alignment for _Interlocked* intrinsics.

Or, if you have suggestions on how to make it easier to review, I'd be open to that.

Version where tests actually pass: I forgot to merge a fixup commit before uploading.

The idea here is not to "ignore type alignment". EmitPointerWithAlignment will sometimes return an alignment for a pointer that's less than the alignment of the pointee type, e.g. because you're taking the address of a packed struct field. The critical question is whether the atomic builtins ought to make an effort to honor that reduced alignment, even if it leads to terrible code, or if we should treat the use of the builtin as a user promise that the pointer is actually more aligned than you might think from the information statically available.

In D97224#2596410, @rjmccall wrote:

Do we really consider the existing atomic intrinsics to not impose added alignment restrictions? I'm somewhat concerned that we're going to produce IR here that's either really suboptimal or, worse, unimplementable, just because we over-interpreted some cue about alignment. I guess it would only be a significant problem on a target where types are frequently under-aligned for what atomics need, which is not typical, or when the user is doing atomics on a field of something like a packed struct.

Address review comments.

Sorry for never looking at this before now. :(

IIUC, the requirement is:

Please reduce the test case per MaskRay's comment, then lg.

In D97268#2583366, @void wrote:

That...shouldn't matter since a NOP is a NOP. Regardless, I removed that modification to simplify the patch.

I can imagine there being some cases where these could theoretically be useful.

In D97268#2582826, @void wrote:

In D97268#2581555, @jyknight wrote:

This change also modifies (breaks!) the 32-bit NOP set, definitely shouldn't do that. (But I think all that can be reverted, in any case).

Which part did it break?

In D97223#2581126, @rjmccall wrote:

Looks like you didn't update the .bc reader/writer and the .ll printer/parser.

This change also modifies (breaks!) the 32-bit NOP set, definitely shouldn't do that. (But I think all that can be reverted, in any case).

Ping.

Ping.

Ping.

I ended up reverting the changes I made to llvm/lib/IR/AutoUpgrade.cpp as the file was including llvm/Analysis/ObjCARCUtil.h, which was violating layering.

This seems to have fallen off everyone's radar -- sorry about that!

This seems to have fallen off everyone's radar -- sorry about that!

Please prefix the title of the commit with "[lld-macho]" so that it's easier to understand what bundle_loader is being implemented in.

In D71726#2537101, @yaxunl wrote:

For amdgpu target, we do need diagnose unsupported atomics (not limited to fp atomics) since we do not support libcall due to ISA level linking not supported. This is something we cannot fix in a short time and we would rather diagnose it than confusing the users with missing symbols in lld.

My concern is that this is treating a backend _bug_ as if it were just an optional feature. But it's not the case that it might be reasonable to either implement or not implement this in a backend -- it should be implemented, and those that don't are buggy.

I still have the same fundamental objection as before to the parts of this patch for prohibiting FP add/sub on some targets.

Add neglected clang-abi-compat.cpp change.

OK, I've posted two follow-up changes now.

Minor updates.

Address comments.

Was the "group" libc++abi accept intended to be an accept for the whole revision? Or should still ping for review from rsmith or someone else?

This doesn't get at the root cause though. Are those labels doing anything in the debug build? Why are they emitted? Can they be reasonably removed?

This patch also fixes the test case in D88438 (and D88438 also fixes this test case).

Looks totally reasonable to me.

Right, there's no fundamental reason why moving a label has to be forbidden -- but it'd be extremely complex if we allowed moving a label which could cause the re-layout of a fragment we thought we'd already finalized the offsets for. This would happen if the label offset required relaxation of some instruction/data referencing it. That, then, might require undoing the padding from an instruction we've already padded out, due to less alignment-padding being required overall.

Agreed w.r.t. timing -- I would like to get all of these changes landed soon after the LLVM 12 branch-cut, to allow plenty time to stew on the main branch before they make it into a release to try to identify any issues.

OK thanks -- abandoning this patch.

In D94268#2485958, @pengfei wrote:

Is inline assembly the only case emms instruction will be needed? But inline assembly doesn't enable mmx attribute automatically, right? E.g. https://godbolt.org/z/43ases

Fix and test.

I've finally got back to moving this patch forward -- PTAL, thanks!

I don't think we should change the meaning of __attribute__((const)) to exclude depending on thread-id.

Oh -- if it's not too late, can you fix the commit message before pushing -- it could probably do with a little more explanation.