Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLVM assumes 4 bytes minimum alignment while the natural alignment on m68k is 2 bytes #10

Open
glaubitz opened this issue Jun 23, 2020 · 27 comments

Comments

@glaubitz
Copy link
Collaborator

Trying to use clang natively on m68k currently fails with a file reading error:

glaubitz@epyc:..llvm-build/bin> uname -a
Linux epyc 5.6.0-2-amd64 #1 SMP Debian 5.6.14-1 (2020-05-23) m68k GNU/Linux
glaubitz@epyc:..llvm-build/bin> ./clang ./hello.c -o hello
error: error reading './hello.c'
1 error generated.
glaubitz@epyc:..llvm-build/bin>

Cross-builds from x86 work fine.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Jul 6, 2020

This might actually be a glibc bug on m68k. It's currently being investigated.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Jul 6, 2020

Okay, this bug is caused by the enforced 32-bit alignment from fcd45ed which breaks the ABI on m68k.

This commit needs to be reverted and then we need to fix the code places where the native 16-bit alignment breaks the LLVM build.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Jul 6, 2020

With the commit reverted, the build on m68k fails due to multiple alignment issues.

Partial build log at: llvm-m68k-native-build.txt

So we need enforce proper alignment on the affected structs when building on m68k.

@glaubitz
Copy link
Collaborator Author

I think one of the root causes might be this snippet from llvm/include/llvm/Support/PointerLikeTypeTraits.h:

template <> struct PointerLikeTypeTraits<void *> {
  static inline void *getAsVoidPointer(void *P) { return P; }
  static inline void *getFromVoidPointer(void *P) { return P; }

  /// Note, we assume here that void* is related to raw malloc'ed memory and                                                                                             
  /// that malloc returns objects at least 4-byte aligned. However, this may be                                                                                          
  /// wrong, or pointers may be from something other than malloc. In this case,                                                                                          
  /// you should specify a real typed pointer or avoid this template.                                                                                                    
  ///                                                                                                                                                                    
  /// All clients should use assertions to do a run-time check to ensure that                                                                                            
  /// this is actually true.                                                                                                                                             
  static constexpr int NumLowBitsAvailable = 2;
};

@glaubitz glaubitz changed the title Running clang natively on m68k fails with error reading source file LLVM assumes 4 bytes minimum alignment while the natural alignment on m68k is 2 bytes Sep 25, 2020
@glaubitz
Copy link
Collaborator Author

glaubitz commented Sep 25, 2020

And the part above is suspicious as well as it uses the alignof() operator which will yield 2 on m68k:

// Provide PointerLikeTypeTraits for non-cvr pointers.                                                                                                                   
template <typename T> struct PointerLikeTypeTraits<T *> {
  static inline void *getAsVoidPointer(T *P) { return P; }
  static inline T *getFromVoidPointer(void *P) { return static_cast<T *>(P); }

  static constexpr int NumLowBitsAvailable =
      detail::ConstantLog2<alignof(T)>::value;
};

@jrtc27
Copy link
Contributor

jrtc27 commented Sep 25, 2020

I think malloc on m68k is more aligned than 2, given glibc uses the following by default:

#define MALLOC_ALIGNMENT (2 * SIZE_SZ < __alignof__ (long double) \
                          ? __alignof__ (long double) : 2 * SIZE_SZ)

Presumably 2 * SIZE_SZ is 8 and __alignof__ (long double) is 2 so malloc uses 8 byte alignment?

@glaubitz
Copy link
Collaborator Author

But would cause 8-byte alignment cause any issues here? I would assume that the LLVM code is happy as long as the minimum alignment is 4 bytes or larger.

@jrtc27
Copy link
Contributor

jrtc27 commented Sep 25, 2020

Yes, it just needs at least 4-byte alignment. The trouble for other pointers (for the non-cvr one) is that I guess some of them could be to the stack and so are only 2-byte aligned? But you could always add attributes to the relevant structs/classes (the ones used with things like PointerIntPair for IntBits > 1) to make them more aligned.

@jrtc27
Copy link
Contributor

jrtc27 commented Sep 25, 2020

(see all the alignas(N) uses in headers and add some more)

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 5, 2020

This trivial workaround seems to fix it and make the build proceed, but it running llvm-tablegen later crashes.

diff --git a/llvm/include/llvm/Support/PointerLikeTypeTraits.h b/llvm/include/llvm/Support/PointerLikeTypeTraits.h
index 1b15f930bd8..406b39cce71 100644
--- a/llvm/include/llvm/Support/PointerLikeTypeTraits.h
+++ b/llvm/include/llvm/Support/PointerLikeTypeTraits.h
@@ -58,7 +58,7 @@ template <typename T> struct PointerLikeTypeTraits<T *> {
   static inline T *getFromVoidPointer(void *P) { return static_cast<T *>(P); }
 
   static constexpr int NumLowBitsAvailable =
-      detail::ConstantLog2<alignof(T)>::value;
+      detail::ConstantLog2<4>::value;
 };
 
 template <> struct PointerLikeTypeTraits<void *> {

I'll try to debug that crash now.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 5, 2020

The crash looks like this:

(gdb) r
Starting program: /local_scratch/M680x0-mono-repo/build/bin/llvm-tblgen -gen-opt-parser-defs -I /local_scratch/M680x0-mono-repo/llvm/tools/llvm-objcopy -I/local_scratch/M680x0-mono-repo/build/include -I/local_scratch/M680x0-mono-repo/llvm/include /local_scratch/M680x0-mono-repo/llvm/tools/llvm-objcopy/ObjcopyOpts.td --write-if-changed -o tools/llvm-objcopy/ObjcopyOpts.inc -d tools/llvm-objcopy/ObjcopyOpts.inc.d
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/m68k-linux-gnu/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x80296052 in llvm::TGParser::SetValue(llvm::Record*, llvm::SMLoc, llvm::Init*, llvm::ArrayRef<unsigned int>, llvm::Init*, bool) ()
(gdb) bt
#0  0x80296052 in llvm::TGParser::SetValue(llvm::Record*, llvm::SMLoc, llvm::Init*, llvm::ArrayRef<unsigned int>, llvm::Init*, bool) ()
#1  0x8029e956 in llvm::TGParser::ParseDeclaration(llvm::Record*, bool) ()
#2  0x8029ea66 in llvm::TGParser::ParseTemplateArgList(llvm::Record*) ()
#3  0x802a4812 in llvm::TGParser::ParseClass() ()
#4  0x802a572a in llvm::TGParser::ParseObjectList(llvm::MultiClass*) ()
#5  0xeffff8e2 in ?? ()
#6  0x00000000 in ?? ()
(gdb)

Will try to rebuild with debug.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 5, 2020

Never mind, the crash in llvm-tblgen actually happens earlier that the compile error with the incorrect alignment.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 6, 2020

I have tried using alignas(4) in several places to fix this problem but that didn't help much. Seems the issue is not trivial to resolve.

diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 7bfc8cfcf1a..679adb4f8c2 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -323,12 +323,6 @@ if(CMAKE_SIZEOF_VOID_P EQUAL 8 AND MINGW)
   add_definitions( -D_FILE_OFFSET_BITS=64 )
 endif()
 
-# GCC m68k on Linux by default aligns on 16bit, we want 32
-if(LLVM_INFERRED_HOST_TRIPLE STREQUAL "m68k-unknown-linux-gnu")
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -malign-int")
-    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -malign-int")
-endif()
-
 if( CMAKE_SIZEOF_VOID_P EQUAL 8 AND NOT WIN32 )
   # TODO: support other platforms and toolchains.
   if( LLVM_BUILD_32_BITS )
diff --git a/llvm/include/llvm/ADT/PointerIntPair.h b/llvm/include/llvm/ADT/PointerIntPair.h
index cb8b202c48b..25e54c31752 100644
--- a/llvm/include/llvm/ADT/PointerIntPair.h
+++ b/llvm/include/llvm/ADT/PointerIntPair.h
@@ -42,7 +42,7 @@ struct PointerIntPairInfo;
 template <typename PointerTy, unsigned IntBits, typename IntType = unsigned,
           typename PtrTraits = PointerLikeTypeTraits<PointerTy>,
           typename Info = PointerIntPairInfo<PointerTy, IntBits, PtrTraits>>
-class PointerIntPair {
+class alignas(4) PointerIntPair {
   // Used by MSVC visualizer and generally helpful for debugging/visualizing.
   using InfoTy = Info;
   intptr_t Value = 0;
@@ -141,7 +141,7 @@ struct is_trivially_copyable<PointerIntPair<PointerTy, IntBits, IntType, PtrTrai
 
 
 template <typename PointerT, unsigned IntBits, typename PtrTraits>
-struct PointerIntPairInfo {
+struct alignas(4) PointerIntPairInfo {
   static_assert(PtrTraits::NumLowBitsAvailable <
                     std::numeric_limits<uintptr_t>::digits,
                 "cannot use a pointer type that has all bits free");
@@ -192,7 +192,7 @@ struct PointerIntPairInfo {
 
 // Provide specialization of DenseMapInfo for PointerIntPair.
 template <typename PointerTy, unsigned IntBits, typename IntType>
-struct DenseMapInfo<PointerIntPair<PointerTy, IntBits, IntType>> {
+struct alignas(4) DenseMapInfo<PointerIntPair<PointerTy, IntBits, IntType>> {
   using Ty = PointerIntPair<PointerTy, IntBits, IntType>;
 
   static Ty getEmptyKey() {
@@ -218,7 +218,7 @@ struct DenseMapInfo<PointerIntPair<PointerTy, IntBits, IntType>> {
 // Teach SmallPtrSet that PointerIntPair is "basically a pointer".
 template <typename PointerTy, unsigned IntBits, typename IntType,
           typename PtrTraits>
-struct PointerLikeTypeTraits<
+struct alignas(4) PointerLikeTypeTraits<
     PointerIntPair<PointerTy, IntBits, IntType, PtrTraits>> {
   static inline void *
   getAsVoidPointer(const PointerIntPair<PointerTy, IntBits, IntType> &P) {
diff --git a/llvm/include/llvm/IR/ValueHandle.h b/llvm/include/llvm/IR/ValueHandle.h
index badc1ca8d1f..58008002a17 100644
--- a/llvm/include/llvm/IR/ValueHandle.h
+++ b/llvm/include/llvm/IR/ValueHandle.h
@@ -46,7 +46,7 @@ protected:
   }
 
 private:
-  PointerIntPair<ValueHandleBase**, 2, HandleBaseKind> PrevPair;
+  PointerIntPair<ValueHandleBase**, 2, HandleBaseKind> alignas(4) PrevPair;
   ValueHandleBase *Next = nullptr;
   Value *Val = nullptr;
 
diff --git a/llvm/include/llvm/MC/MCSymbol.h b/llvm/include/llvm/MC/MCSymbol.h
index a89de5dfa22..abdaeb9d0f8 100644
--- a/llvm/include/llvm/MC/MCSymbol.h
+++ b/llvm/include/llvm/MC/MCSymbol.h
@@ -38,7 +38,7 @@ class raw_ostream;
 /// If the symbol is defined/emitted into the current translation unit, the
 /// Section member is set to indicate what section it lives in.  Otherwise, if
 /// it is a reference to an external entity, it has a null section.
-class MCSymbol {
+class alignas(4) MCSymbol {
 protected:
   /// The kind of the symbol.  If it is any value other than unset then this
   /// class is actually one of the appropriate subclasses of MCSymbol.
@@ -146,7 +146,7 @@ protected:
   /// MCSymbol contains a uint64_t so is probably aligned to 8.  On a 32-bit
   /// system, the name is a pointer so isn't going to satisfy the 8 byte
   /// alignment of uint64_t.  Account for that here.
-  using NameEntryStorageTy = union {
+  using NameEntryStorageTy = union alignas(4) {
     const StringMapEntry<bool> *NameEntry;
     uint64_t AlignmentPadding;
   };
diff --git a/llvm/include/llvm/Support/PointerLikeTypeTraits.h b/llvm/include/llvm/Support/PointerLikeTypeTraits.h
index 1b15f930bd8..58d2cc00ebb 100644
--- a/llvm/include/llvm/Support/PointerLikeTypeTraits.h
+++ b/llvm/include/llvm/Support/PointerLikeTypeTraits.h
@@ -53,7 +53,7 @@ template <typename T> struct IsPointerLike<T *> {
 } // namespace detail
 
 // Provide PointerLikeTypeTraits for non-cvr pointers.
-template <typename T> struct PointerLikeTypeTraits<T *> {
+  template <typename T> struct alignas(4) PointerLikeTypeTraits<T *> {
   static inline void *getAsVoidPointer(T *P) { return P; }
   static inline T *getFromVoidPointer(void *P) { return static_cast<T *>(P); }
 
@@ -61,7 +61,7 @@ template <typename T> struct PointerLikeTypeTraits<T *> {
       detail::ConstantLog2<alignof(T)>::value;
 };
 
-template <> struct PointerLikeTypeTraits<void *> {
+template <> struct alignas(4) PointerLikeTypeTraits<void *> {
   static inline void *getAsVoidPointer(void *P) { return P; }
   static inline void *getFromVoidPointer(void *P) { return P; }
 
@@ -76,7 +76,7 @@ template <> struct PointerLikeTypeTraits<void *> {
 };
 
 // Provide PointerLikeTypeTraits for const things.
-template <typename T> struct PointerLikeTypeTraits<const T> {
+template <typename T> struct alignas(4) PointerLikeTypeTraits<const T> {
   typedef PointerLikeTypeTraits<T> NonConst;
 
   static inline const void *getAsVoidPointer(const T P) {
@@ -89,7 +89,7 @@ template <typename T> struct PointerLikeTypeTraits<const T> {
 };
 
 // Provide PointerLikeTypeTraits for const pointers.
-template <typename T> struct PointerLikeTypeTraits<const T *> {
+template <typename T> struct alignas(4) PointerLikeTypeTraits<const T *> {
   typedef PointerLikeTypeTraits<T *> NonConst;
 
   static inline const void *getAsVoidPointer(const T *P) {
@@ -102,7 +102,7 @@ template <typename T> struct PointerLikeTypeTraits<const T *> {
 };
 
 // Provide PointerLikeTypeTraits for uintptr_t.
-template <> struct PointerLikeTypeTraits<uintptr_t> {
+template <> struct alignas(4) PointerLikeTypeTraits<uintptr_t> {
   static inline void *getAsVoidPointer(uintptr_t P) {
     return reinterpret_cast<void *>(P);
   }
@@ -122,7 +122,7 @@ template <> struct PointerLikeTypeTraits<uintptr_t> {
 /// customized form of this template explicitly with higher alignment, and
 /// potentially use alignment attributes on functions to satisfy that.
 template <int Alignment, typename FunctionPointerT>
-struct FunctionPointerLikeTypeTraits {
+struct alignas(4) FunctionPointerLikeTypeTraits {
   static constexpr int NumLowBitsAvailable =
       detail::ConstantLog2<Alignment>::value;
   static inline void *getAsVoidPointer(FunctionPointerT P) {

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 6, 2020

Another possible fix would be what @karcherm suggested, include C headers files with separate alignment, i.e.:

--- a/llvm/include/llvm/Support/FileSystem.h
+++ b/llvm/include/llvm/Support/FileSystem.h
@@ -45,9 +45,11 @@
 #include <tuple>
 #include <vector>
 
+#pragma pack(2)
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
+#pragma pack()

I'm giving this a try now.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 6, 2020

It actually works now:

(sid-m68k-sbuild)root@epyc:/local_scratch/M680x0-mono-repo/build# ./bin/clang hello.c -o hello
(sid-m68k-sbuild)root@epyc:/local_scratch/M680x0-mono-repo/build# ./hello
Hello World!
(sid-m68k-sbuild)root@epyc:/local_scratch/M680x0-mono-repo/build#

With this change:

diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h
index 5a8842ee26e..450dfe11dd9 100644
--- a/llvm/include/llvm/Support/FileSystem.h
+++ b/llvm/include/llvm/Support/FileSystem.h
@@ -45,9 +45,11 @@
 #include <tuple>
 #include <vector>
 
+#pragma pack(2)
 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
+#pragma pack()
 
 namespace llvm {
 namespace sys {
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 01903ea10e8..9e385d156a0 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -15,6 +15,7 @@
 //===          is guaranteed to work on *all* UNIX variants.
 //===----------------------------------------------------------------------===//
 
+#pragma pack(2)
 #include "Unix.h"
 #include <limits.h>
 #include <stdio.h>
@@ -52,6 +53,7 @@ extern char **environ;
 #elif defined(__MVS__)
 #include <sys/ps.h>
 #endif
+#pragma pack()
 
 // Both stdio.h and cstdio are included via different paths and
 // stdcxx's cstdio doesn't include stdio.h, so it doesn't #undef the macros

Maybe even the first #pragma is already enough.

@jrtc27
Copy link
Contributor

jrtc27 commented Oct 6, 2020

Not a fix, very fragile and will eventually break (and definitely not upstreamable).

@mshockwave
Copy link
Member

Thank you @glaubitz for the exploration :-) It's a non-trivial problem for sure

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 6, 2020

Not a fix, very fragile and will eventually break (and definitely not upstreamable).

We would be guarding that with #if defined(__m68k__) ... #endif, of course. And if we're lucky, it affects <sys/stat.h> only.

My wild guess is that the C++ ABI works with either alignments and the C ABI has some places where it breaks.

In particular, the affected data type was struct stat64 as the offset of __mode_t st_mode; is shifted when alignment is set to 32 bits due to the extra padding unsigned short int __pad1;.

See: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/m68k/bits/stat.h;h=453dcac709d54d639dd4b5fcde64b4d9f67bfd2d;hb=HEAD#l94

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 7, 2020

I'm using this variant of the patch now which works just fine:

diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 01903ea10e8..11c2953d795 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -15,6 +15,10 @@
 //===          is guaranteed to work on *all* UNIX variants.
 //===----------------------------------------------------------------------===//
 
+#ifdef __m68k__
+#pragma pack (2)
+#endif
+
 #include "Unix.h"
 #include <limits.h>
 #include <stdio.h>
@@ -53,6 +57,10 @@ extern char **environ;
 #include <sys/ps.h>
 #endif
 
+#ifdef __m68k__
+#pragma pack ()
+#endif
+
 // Both stdio.h and cstdio are included via different paths and
 // stdcxx's cstdio doesn't include stdio.h, so it doesn't #undef the macros
 // either.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 7, 2020

I'm trying to reduce the patch to the offending header now. Maybe this is something that can be fixed in glibc, i.e. in order to make glibc robust for code with 32-bit alignment.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Oct 7, 2020

Okay, it turns out it actually affects the inclusion of fcntl.h in Unix.h.

This is the smallest change that makes it work:

diff --git a/llvm/lib/Support/Unix/Unix.h b/llvm/lib/Support/Unix/Unix.h
index 60929139598..0f7e3b03885 100644
--- a/llvm/lib/Support/Unix/Unix.h
+++ b/llvm/lib/Support/Unix/Unix.h
@@ -45,10 +45,18 @@
 # include <dlfcn.h>
 #endif
 
+#ifdef __m68k__
+#pragma pack (2)
+#endif
+
 #ifdef HAVE_FCNTL_H
 # include <fcntl.h>
 #endif
 
+#ifdef __m68k__
+#pragma pack ()
+#endif
+
 /// This function builds an error message into \p ErrMsg using the \p prefix
 /// string and the Unix error number given b

@glaubitz
Copy link
Collaborator Author

I have forwarded the issue to glibc upstream where I'm asking for advice: https://sourceware.org/bugzilla/show_bug.cgi?id=26724

@glaubitz
Copy link
Collaborator Author

glaubitz commented Sep 1, 2021

I'm currently trying to build LLVM natively on Debian/m68k and I'm using the minimal patch from the comment above.

However, it seems that we need to force 2-byte alignment now at someplace else since TableGen is now confused with 4-byte alignment on m68k:

[ 26%] Building arm_bf16.h...
[ 26%] Building arm_cde.h...
cd "/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers" && ../../../../bin/clang-tblgen -gen-arm-sve-header -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/ -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/lib/Headers -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/llvm/include /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_sve.td --write-if-changed -o /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers/arm_sve.h
cd "/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers" && ../../../../bin/clang-tblgen -gen-arm-mve-header -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/ -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/lib/Headers -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/llvm/include /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_mve.td --write-if-changed -o /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers/arm_mve.h
cd "/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers" && ../../../../bin/clang-tblgen -gen-arm-bf16 -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/ -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/lib/Headers -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/llvm/include /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_bf16.td --write-if-changed -o /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers/arm_bf16.h
cd "/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers" && ../../../../bin/clang-tblgen -gen-arm-cde-header -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/ -I /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/lib/Headers -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/include -I/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/llvm/include /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_cde.td --write-if-changed -o /build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm/tools/clang/lib/Headers/arm_cde.h
/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_bf16.td:1:81: error: Unexpected token at top level
//===--- arm_fp16.td - ARM BF16 compiler interface ------------------------===//
                                                                                ^
make[2]: *** [tools/clang/lib/Headers/CMakeFiles/clang-resource-headers.dir/build.make:1084: tools/clang/lib/Headers/arm_bf16.h] Error 1
/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_fp16.td:1:81: error: Unexpected token at top level
//===--- arm_fp16.td - ARM FP16 compiler interface ------------------------===//
                                                                                ^
/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/riscv_vector.td:1:81: error: Unexpected token at top level
/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_mve.td://==--- riscv_vector.td - RISC-V V-ext Builtin function list --------------===//
                   1                         :              81            :           ^
error: Unexpected token at top level
//===- arm_mve.td - ACLE intrinsic functions for MVE architecture ---------===//
                                                                                ^
make[2]: *** [tools/clang/lib/Headers/CMakeFiles/clang-resource-headers.dir/build.make:996: tools/clang/lib/Headers/arm_fp16.h] Error 1
/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_cde.td:1:81: error: Unexpected token at top level
//===--- arm_cde.td - ACLE intrinsic functions for CDE --------------------===//
                                                                                ^
/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/clang/include/clang/Basic/arm_sve.td:make[2]: *** [tools/clang/lib/Headers/CMakeFiles/clang-resource-headers.dir/build.make:1216: tools/clang/lib/Headers/riscv_vector.h] Error 1
1:79: error: Unexpected token at top level
//===--- arm_sve.td - ARM SVE compiler interface ------------------------===//
                                                                              ^
make[2]: *** [tools/clang/lib/Headers/CMakeFiles/clang-resource-headers.dir/build.make:1128: tools/clang/lib/Headers/arm_mve.h] Error 1
make[2]: *** [tools/clang/lib/Headers/CMakeFiles/clang-resource-headers.dir/build.make:1172: tools/clang/lib/Headers/arm_cde.h] Error 1
make[2]: *** [tools/clang/lib/Headers/CMakeFiles/clang-resource-headers.dir/build.make:1040: tools/clang/lib/Headers/arm_sve.h] Error 1
make[2]: Leaving directory '/build/llvm-toolchain-13-7X37DV/llvm-toolchain-13-13.0.0~+rc1/build-llvm'
make[1]: *** [CMakeFiles/Makefile2:37302: tools/clang/lib/Headers/CMakeFiles/clang-resource-headers.dir/all] Error 2

I have not yet figured out what C header here needs 2-byte alignment.

mshockwave pushed a commit that referenced this issue Feb 7, 2022
We experienced some deadlocks when we used multiple threads for logging
using `scan-builds` intercept-build tool when we used multiple threads by
e.g. logging `make -j16`

```
(gdb) bt
#0  0x00007f2bb3aff110 in __lll_lock_wait () from /lib/x86_64-linux-gnu/libpthread.so.0
#1  0x00007f2bb3af70a3 in pthread_mutex_lock () from /lib/x86_64-linux-gnu/libpthread.so.0
#2  0x00007f2bb3d152e4 in ?? ()
#3  0x00007ffcc5f0cc80 in ?? ()
#4  0x00007f2bb3d2bf5b in ?? () from /lib64/ld-linux-x86-64.so.2
#5  0x00007f2bb3b5da27 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#6  0x00007f2bb3b5dbe0 in exit () from /lib/x86_64-linux-gnu/libc.so.6
#7  0x00007f2bb3d144ee in ?? ()
#8  0x746e692f706d742f in ?? ()
#9  0x692d747065637265 in ?? ()
#10 0x2f653631326b3034 in ?? ()
#11 0x646d632e35353532 in ?? ()
#12 0x0000000000000000 in ?? ()
```

I think the gcc's exit call caused the injected `libear.so` to be unloaded
by the `ld`, which in turn called the `void on_unload() __attribute__((destructor))`.
That tried to acquire an already locked mutex which was left locked in the
`bear_report_call()` call, that probably encountered some error and
returned early when it forgot to unlock the mutex.

All of these are speculation since from the backtrace I could not verify
if frames 2 and 3 are in fact corresponding to the `libear.so` module.
But I think it's a fairly safe bet.

So, hereby I'm releasing the held mutex on *all paths*, even if some failure
happens.

PS: I would use lock_guards, but it's C.

Reviewed-by: NoQ

Differential Revision: https://reviews.llvm.org/D118439
@glaubitz
Copy link
Collaborator Author

FWIW, there are discussions in Debian to rebootstrap 32-bit architectures in the future to solve the Y2038 problem, see:

https://lists.debian.org/debian-arm/2022/10/msg00020.html

We could use this opportunity to switch the m68k port over to 32-bit alignment since the above rebootstrap would break the ABI anyway.

If we rebootstrap with 32-bit alignment, we would automatically solve the alignment problem once and for all.

@chewi
Copy link

chewi commented Oct 22, 2022

I had considered whether to do this on Gentoo too. Shame we didn't do it when we started producing the new stage tarballs last year. I take it the downside is more memory usage? That would be unfortunate on this architecture. My Amiga has 128MB, but even that's more than most.

@glaubitz
Copy link
Collaborator Author

I assume we would have to patch glibc in any case since the glibc ABI assumes 16-bit alignment.

@karcherm
Copy link

I take it the downside is more memory usage? That would be unfortunate on this architecture.

Extra alignment obviously has this downside. On the other hand, the downside is likely small. All other 32-bit architectures of the time (amongst other: i386) did choose 32-bit alignment, just as John Paul Adrian proposes for m68k. A typical i386-class PC compatible mainboard between 1990 and 1994 had 8 30-pin memory slots, and supported 4MB/slot at max, so they were maxed out at 32MB, but no one proposed to get rid of 32-bit alignment for less memory usage. Also consider the upside: With 32-bit alignment, all 32-bit variable accesses are aligned, and take just a single bus cycle, whereas with 16-bit alignment, you get to pay a performance penalty for every 32-bit value that is misaligned. So in the end, this is a memory usage/speed tradeoff, and not only memory is a limiting factor on Amiga computers, but CPU power / bus performance also is a bottleneck from today's point of view.

As I understand it, the primary reason for m68k not using 32-bit alignment is binary compatibility with early m68k code. The original 68000 and 68010 processors had a 16-bit data bus (thus the name Atari ST for Sixtenn/Thirty-Two), so defaulting everything to 16-bit alignment made a lot of sense when the ABI for those processors was established. When the Motorola series got the 32-bit bus, a lot of 32-bit code with 16-bit alignment was already in the wild. On the other hand, when Intel introduced the 386DX, no Intel 32-bit code was in the wild at all.

As we are breaking binary compatibility anyway when we redefine time_t to have 64-bit, the primary reason for 16-bit alignment is void, so getting m68k more in line with all the other 32-bit architectures from the late 80s and early 90s makes a lot of sense and reduces workload on retro architecture maintainers.

mshockwave pushed a commit that referenced this issue Nov 14, 2022
Found by msan -fsanitize-memory-use-after-dtor.

==8259==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x55dbec54d2b8 in dtorRecord(clang::interp::Block*, char*, clang::interp::Descriptor*) clang/lib/AST/Interp/Descriptor.cpp:150:22
    #1 0x55dbec54bfcf in dtorArrayDesc(clang::interp::Block*, char*, clang::interp::Descriptor*) clang/lib/AST/Interp/Descriptor.cpp:97:7
    #2 0x55dbec508578 in invokeDtor clang/lib/AST/Interp/InterpBlock.h:79:7
    #3 0x55dbec508578 in clang::interp::Program::~Program() clang/lib/AST/Interp/Program.h:55:19
    #4 0x55dbec50657a in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    #5 0x55dbec50657a in std::__msan::unique_ptr<clang::interp::Program, std::__msan::default_delete<clang::interp::Program>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    #6 0x55dbec5035a1 in clang::interp::Context::~Context() clang/lib/AST/Interp/Context.cpp:27:22
    #7 0x55dbebec1daa in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    #8 0x55dbebec1daa in std::__msan::unique_ptr<clang::interp::Context, std::__msan::default_delete<clang::interp::Context>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    #9 0x55dbebe285f9 in clang::ASTContext::~ASTContext() clang/lib/AST/ASTContext.cpp:1038:40
    #10 0x55dbe941ff13 in llvm::RefCountedBase<clang::ASTContext>::Release() const llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:101:7
    #11 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:159:38
    #12 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:224:7
    #13 0x55dbe94353ef in ~IntrusiveRefCntPtr llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:191:27
    #14 0x55dbe94353ef in clang::CompilerInstance::setASTContext(clang::ASTContext*) clang/lib/Frontend/CompilerInstance.cpp:178:3
    #15 0x55dbe95ad0ad in clang::FrontendAction::EndSourceFile() clang/lib/Frontend/FrontendAction.cpp:1100:8
    #16 0x55dbe9445fcf in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) clang/lib/Frontend/CompilerInstance.cpp:1047:11
    #17 0x55dbe6b3afef in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
    #18 0x55dbe6b13288 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) clang/tools/driver/cc1_main.cpp:250:15
    #19 0x55dbe6b0095f in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) clang/tools/driver/driver.cpp:319:12
    #20 0x55dbe6aff41c in clang_main(int, char**) clang/tools/driver/driver.cpp:395:12
    #21 0x7f9be07fa632 in __libc_start_main
    #22 0x55dbe6a702e9 in _start

  Member fields were destroyed
    #0 0x55dbe6a7da5d in __sanitizer_dtor_callback_fields compiler-rt/lib/msan/msan_interceptors.cpp:949:5
    #1 0x55dbec5094ac in ~SmallVectorImpl llvm/include/llvm/ADT/SmallVector.h:479:7
    #2 0x55dbec5094ac in ~SmallVectorImpl llvm/include/llvm/ADT/SmallVector.h:612:3
    #3 0x55dbec5094ac in llvm::SmallVector<clang::interp::Record::Base, 8u>::~SmallVector() llvm/include/llvm/ADT/SmallVector.h:1207:3
    #4 0x55dbec508e79 in clang::interp::Record::~Record() clang/lib/AST/Interp/Record.h:24:7
    #5 0x55dbec508612 in clang::interp::Program::~Program() clang/lib/AST/Interp/Program.h:49:26
    #6 0x55dbec50657a in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    #7 0x55dbec50657a in std::__msan::unique_ptr<clang::interp::Program, std::__msan::default_delete<clang::interp::Program>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    #8 0x55dbec5035a1 in clang::interp::Context::~Context() clang/lib/AST/Interp/Context.cpp:27:22
    #9 0x55dbebec1daa in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
    #10 0x55dbebec1daa in std::__msan::unique_ptr<clang::interp::Context, std::__msan::default_delete<clang::interp::Context>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
    #11 0x55dbebe285f9 in clang::ASTContext::~ASTContext() clang/lib/AST/ASTContext.cpp:1038:40
    #12 0x55dbe941ff13 in llvm::RefCountedBase<clang::ASTContext>::Release() const llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:101:7
    #13 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:159:38
    #14 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:224:7
    #15 0x55dbe94353ef in ~IntrusiveRefCntPtr llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:191:27
    #16 0x55dbe94353ef in clang::CompilerInstance::setASTContext(clang::ASTContext*) clang/lib/Frontend/CompilerInstance.cpp:178:3
    #17 0x55dbe95ad0ad in clang::FrontendAction::EndSourceFile() clang/lib/Frontend/FrontendAction.cpp:1100:8
    #18 0x55dbe9445fcf in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) clang/lib/Frontend/CompilerInstance.cpp:1047:11
    #19 0x55dbe6b3afef in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
    #20 0x55dbe6b13288 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) clang/tools/driver/cc1_main.cpp:250:15
    #21 0x55dbe6b0095f in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) clang/tools/driver/driver.cpp:319:12
    #22 0x55dbe6aff41c in clang_main(int, char**) clang/tools/driver/driver.cpp:395:12
    #23 0x7f9be07fa632 in __libc_start_main
    #24 0x55dbe6a702e9 in _start
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants