Skip to content

Conversation

@rorth
Copy link
Collaborator

@rorth rorth commented Jul 23, 2024

As discussed in Issue #86793, the MachOTest.UnalignedLC test dies with SIGBUS on SPARC, a strict-alignment target. It simply cannot work there. Besides, the test invokes undefined behaviour on big-endian targets, so this patch disables it on all of those.

Tested on sparcv9-sun-solaris2.11 and amd64-pc-solaris2.11.

As discussed in Issue llvm#86793, the `MachOTest.UnalignedLC` test dies with
SIGBUS on SPARC, a strict-alignment target.  It simply cannot work there,
so this patch disables it.

Given that there's doubt about the test's correctnes, one might consider
disabling it wholesale.

Tested on `sparcv9-sun-solaris2.11` and `amd64-pc-solaris2.11`.
@rorth rorth requested a review from efriedma-quic July 23, 2024 09:13
@efriedma-quic
Copy link
Collaborator

Given the test has undefined behavior on all big-endian hosts, I'd prefer to do something to fix the issue for all big-endian hosts, not just SPARC.

@rorth rorth added this to the LLVM 19.X Release milestone Jul 29, 2024
@rorth
Copy link
Collaborator Author

rorth commented Jul 30, 2024

Disabled now on big-endian in general. __BYTE_ORDER__ etc. are predefined on clang and gcc, but I'm uncertain if the test needs to be compilable by other compilers.

@efriedma-quic
Copy link
Collaborator

llvm/include/llvm/ADT/bit.h provides define BYTE_ORDER and BIG_ENDIAN on all targets.

@rorth
Copy link
Collaborator Author

rorth commented Aug 2, 2024

I'd seen that, but based on

/// \file
/// This file implements the C++20 <bit> header.

I was under the impression that those are implementation details that cannot be relied on.

@github-actions
Copy link

github-actions bot commented Aug 2, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 430b2545032db9de7898444502915f89e20f7c4c d238831d828149fb396d703f601c9783bcc37ba6 --extensions cpp -- llvm/unittests/BinaryFormat/MachOTest.cpp
View the diff from clang-format here.
diff --git a/llvm/unittests/BinaryFormat/MachOTest.cpp b/llvm/unittests/BinaryFormat/MachOTest.cpp
index 78b20c28a9..46f42f4b7c 100644
--- a/llvm/unittests/BinaryFormat/MachOTest.cpp
+++ b/llvm/unittests/BinaryFormat/MachOTest.cpp
@@ -6,8 +6,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/ADT/bit.h"
 #include "llvm/BinaryFormat/MachO.h"
+#include "llvm/ADT/bit.h"
 #include "llvm/TargetParser/Triple.h"
 #include "gtest/gtest.h"
 

@MaskRay MaskRay requested a review from llvm-beanz August 2, 2024 17:38
Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rorth rorth merged commit 3a226db into llvm:main Aug 6, 2024
@rorth
Copy link
Collaborator Author

rorth commented Aug 6, 2024

/cherry-pick 3a226db

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 6, 2024
As discussed in Issue llvm#86793, the `MachOTest.UnalignedLC` test dies with
`SIGBUS` on SPARC, a strict-alignment target. It simply cannot work
there. Besides, the test invokes undefined behaviour on big-endian
targets, so this patch disables it on all of those.

Tested on `sparcv9-sun-solaris2.11` and `amd64-pc-solaris2.11`.

(cherry picked from commit 3a226db)
@llvmbot
Copy link
Member

llvmbot commented Aug 6, 2024

/pull-request #102103

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 10, 2024
As discussed in Issue llvm#86793, the `MachOTest.UnalignedLC` test dies with
`SIGBUS` on SPARC, a strict-alignment target. It simply cannot work
there. Besides, the test invokes undefined behaviour on big-endian
targets, so this patch disables it on all of those.

Tested on `sparcv9-sun-solaris2.11` and `amd64-pc-solaris2.11`.

(cherry picked from commit 3a226db)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants