rpms / llvm

Clone
Source Code
GIT

#174 Improve clang assembler error messages on s390x (rhbz#2216906)
Merged 2 months ago by tuliom. Opened 2 months ago by tuliom.
rpms/ tuliom/llvm llvm_unreacheable  into  rawhide

file added
+184 
@@ -0,0 +1,184 @@ 
 

+ From efbaf8bc61f4c0e29a3eaafb11ac0ddda8bd3dff Mon Sep 17 00:00:00 2001
 

+ From: Ulrich Weigand <ulrich.weigand@de.ibm.com>
 

+ Date: Fri, 30 Jun 2023 16:02:56 +0200
 

+ Subject: [PATCH] [SystemZ] Improve error messages for unsupported relocations
 

+ 

+ In the SystemZMCObjectWriter, we currently just abort in case
 

+ some unsupported relocation in requested.  However, as this
 

+ situation can be triggered by invalid (inline) assembler input,
 

+ we should really get a regular error message instead.
 

+ ---
 

+  .../MCTargetDesc/SystemZMCObjectWriter.cpp    | 59 +++++++++++--------
 

+  1 file changed, 35 insertions(+), 24 deletions(-)
 

+ 

+ diff --git a/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp b/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
 

+ index c23463ab9bde..0b11468afc52 100644
 

+ --- a/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
 

+ +++ b/llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCObjectWriter.cpp
 

+ @@ -9,6 +9,7 @@
 

+  #include "MCTargetDesc/SystemZMCFixups.h"
 

+  #include "MCTargetDesc/SystemZMCTargetDesc.h"
 

+  #include "llvm/BinaryFormat/ELF.h"
 

+ +#include "llvm/MC/MCContext.h"
 

+  #include "llvm/MC/MCELFObjectWriter.h"
 

+  #include "llvm/MC/MCExpr.h"
 

+  #include "llvm/MC/MCFixup.h"
 

+ @@ -40,7 +41,7 @@ SystemZObjectWriter::SystemZObjectWriter(uint8_t OSABI)
 

+                              /*HasRelocationAddend_=*/ true) {}
 

+  

+  // Return the relocation type for an absolute value of MCFixupKind Kind.
 

+ -static unsigned getAbsoluteReloc(unsigned Kind) {
 

+ +static unsigned getAbsoluteReloc(MCContext &Ctx, SMLoc Loc, unsigned Kind) {
 

+    switch (Kind) {
 

+    case FK_Data_1: return ELF::R_390_8;
 

+    case FK_Data_2: return ELF::R_390_16;
 

+ @@ -49,11 +50,12 @@ static unsigned getAbsoluteReloc(unsigned Kind) {
 

+    case SystemZ::FK_390_12: return ELF::R_390_12;
 

+    case SystemZ::FK_390_20: return ELF::R_390_20;
 

+    }
 

+ -  llvm_unreachable("Unsupported absolute address");
 

+ +  Ctx.reportError(Loc, "Unsupported absolute address");
 

+ +  return 0;
 

+  }
 

+  

+  // Return the relocation type for a PC-relative value of MCFixupKind Kind.
 

+ -static unsigned getPCRelReloc(unsigned Kind) {
 

+ +static unsigned getPCRelReloc(MCContext &Ctx, SMLoc Loc, unsigned Kind) {
 

+    switch (Kind) {
 

+    case FK_Data_2:                return ELF::R_390_PC16;
 

+    case FK_Data_4:                return ELF::R_390_PC32;
 

+ @@ -63,62 +65,69 @@ static unsigned getPCRelReloc(unsigned Kind) {
 

+    case SystemZ::FK_390_PC24DBL:  return ELF::R_390_PC24DBL;
 

+    case SystemZ::FK_390_PC32DBL:  return ELF::R_390_PC32DBL;
 

+    }
 

+ -  llvm_unreachable("Unsupported PC-relative address");
 

+ +  Ctx.reportError(Loc, "Unsupported PC-relative address");
 

+ +  return 0;
 

+  }
 

+  

+  // Return the R_390_TLS_LE* relocation type for MCFixupKind Kind.
 

+ -static unsigned getTLSLEReloc(unsigned Kind) {
 

+ +static unsigned getTLSLEReloc(MCContext &Ctx, SMLoc Loc, unsigned Kind) {
 

+    switch (Kind) {
 

+    case FK_Data_4: return ELF::R_390_TLS_LE32;
 

+    case FK_Data_8: return ELF::R_390_TLS_LE64;
 

+    }
 

+ -  llvm_unreachable("Unsupported absolute address");
 

+ +  Ctx.reportError(Loc, "Unsupported thread-local address (local-exec)");
 

+ +  return 0;
 

+  }
 

+  

+  // Return the R_390_TLS_LDO* relocation type for MCFixupKind Kind.
 

+ -static unsigned getTLSLDOReloc(unsigned Kind) {
 

+ +static unsigned getTLSLDOReloc(MCContext &Ctx, SMLoc Loc, unsigned Kind) {
 

+    switch (Kind) {
 

+    case FK_Data_4: return ELF::R_390_TLS_LDO32;
 

+    case FK_Data_8: return ELF::R_390_TLS_LDO64;
 

+    }
 

+ -  llvm_unreachable("Unsupported absolute address");
 

+ +  Ctx.reportError(Loc, "Unsupported thread-local address (local-dynamic)");
 

+ +  return 0;
 

+  }
 

+  

+  // Return the R_390_TLS_LDM* relocation type for MCFixupKind Kind.
 

+ -static unsigned getTLSLDMReloc(unsigned Kind) {
 

+ +static unsigned getTLSLDMReloc(MCContext &Ctx, SMLoc Loc, unsigned Kind) {
 

+    switch (Kind) {
 

+    case FK_Data_4: return ELF::R_390_TLS_LDM32;
 

+    case FK_Data_8: return ELF::R_390_TLS_LDM64;
 

+    case SystemZ::FK_390_TLS_CALL: return ELF::R_390_TLS_LDCALL;
 

+    }
 

+ -  llvm_unreachable("Unsupported absolute address");
 

+ +  Ctx.reportError(Loc, "Unsupported thread-local address (local-dynamic)");
 

+ +  return 0;
 

+  }
 

+  

+  // Return the R_390_TLS_GD* relocation type for MCFixupKind Kind.
 

+ -static unsigned getTLSGDReloc(unsigned Kind) {
 

+ +static unsigned getTLSGDReloc(MCContext &Ctx, SMLoc Loc, unsigned Kind) {
 

+    switch (Kind) {
 

+    case FK_Data_4: return ELF::R_390_TLS_GD32;
 

+    case FK_Data_8: return ELF::R_390_TLS_GD64;
 

+    case SystemZ::FK_390_TLS_CALL: return ELF::R_390_TLS_GDCALL;
 

+    }
 

+ -  llvm_unreachable("Unsupported absolute address");
 

+ +  Ctx.reportError(Loc, "Unsupported thread-local address (general-dynamic)");
 

+ +  return 0;
 

+  }
 

+  

+  // Return the PLT relocation counterpart of MCFixupKind Kind.
 

+ -static unsigned getPLTReloc(unsigned Kind) {
 

+ +static unsigned getPLTReloc(MCContext &Ctx, SMLoc Loc, unsigned Kind) {
 

+    switch (Kind) {
 

+    case SystemZ::FK_390_PC12DBL: return ELF::R_390_PLT12DBL;
 

+    case SystemZ::FK_390_PC16DBL: return ELF::R_390_PLT16DBL;
 

+    case SystemZ::FK_390_PC24DBL: return ELF::R_390_PLT24DBL;
 

+    case SystemZ::FK_390_PC32DBL: return ELF::R_390_PLT32DBL;
 

+    }
 

+ -  llvm_unreachable("Unsupported absolute address");
 

+ +  Ctx.reportError(Loc, "Unsupported PC-relative PLT address");
 

+ +  return 0;
 

+  }
 

+  

+  unsigned SystemZObjectWriter::getRelocType(MCContext &Ctx,
 

+                                             const MCValue &Target,
 

+                                             const MCFixup &Fixup,
 

+                                             bool IsPCRel) const {
 

+ +  SMLoc Loc = Fixup.getLoc();
 

+    unsigned Kind = Fixup.getKind();
 

+    if (Kind >= FirstLiteralRelocationKind)
 

+      return Kind - FirstLiteralRelocationKind;
 

+ @@ -126,38 +135,40 @@ unsigned SystemZObjectWriter::getRelocType(MCContext &Ctx,
 

+    switch (Modifier) {
 

+    case MCSymbolRefExpr::VK_None:
 

+      if (IsPCRel)
 

+ -      return getPCRelReloc(Kind);
 

+ -    return getAbsoluteReloc(Kind);
 

+ +      return getPCRelReloc(Ctx, Loc, Kind);
 

+ +    return getAbsoluteReloc(Ctx, Loc, Kind);
 

+  

+    case MCSymbolRefExpr::VK_NTPOFF:
 

+      assert(!IsPCRel && "NTPOFF shouldn't be PC-relative");
 

+ -    return getTLSLEReloc(Kind);
 

+ +    return getTLSLEReloc(Ctx, Loc, Kind);
 

+  

+    case MCSymbolRefExpr::VK_INDNTPOFF:
 

+      if (IsPCRel && Kind == SystemZ::FK_390_PC32DBL)
 

+        return ELF::R_390_TLS_IEENT;
 

+ -    llvm_unreachable("Only PC-relative INDNTPOFF accesses are supported for now");
 

+ +    Ctx.reportError(Loc, "Only PC-relative INDNTPOFF accesses are supported for now");
 

+ +    return 0;
 

+  

+    case MCSymbolRefExpr::VK_DTPOFF:
 

+      assert(!IsPCRel && "DTPOFF shouldn't be PC-relative");
 

+ -    return getTLSLDOReloc(Kind);
 

+ +    return getTLSLDOReloc(Ctx, Loc, Kind);
 

+  

+    case MCSymbolRefExpr::VK_TLSLDM:
 

+      assert(!IsPCRel && "TLSLDM shouldn't be PC-relative");
 

+ -    return getTLSLDMReloc(Kind);
 

+ +    return getTLSLDMReloc(Ctx, Loc, Kind);
 

+  

+    case MCSymbolRefExpr::VK_TLSGD:
 

+      assert(!IsPCRel && "TLSGD shouldn't be PC-relative");
 

+ -    return getTLSGDReloc(Kind);
 

+ +    return getTLSGDReloc(Ctx, Loc, Kind);
 

+  

+    case MCSymbolRefExpr::VK_GOT:
 

+      if (IsPCRel && Kind == SystemZ::FK_390_PC32DBL)
 

+        return ELF::R_390_GOTENT;
 

+ -    llvm_unreachable("Only PC-relative GOT accesses are supported for now");
 

+ +    Ctx.reportError(Loc, "Only PC-relative GOT accesses are supported for now");
 

+ +    return 0;
 

+  

+    case MCSymbolRefExpr::VK_PLT:
 

+ -    assert(IsPCRel && "@PLT shouldt be PC-relative");
 

+ -    return getPLTReloc(Kind);
 

+ +    assert(IsPCRel && "@PLT shouldn't be PC-relative");
 

+ +    return getPLTReloc(Ctx, Loc, Kind);
 

+  

+    default:
 

+      llvm_unreachable("Modifier not supported");
 

+ -- 

+ 2.41.0
 

+

file modified
+8 -1 
@@ -77,7 +77,7 @@ 
 

  

  Name:		%{pkg_name}
 

  Version:	%{maj_ver}.%{min_ver}.%{patch_ver}%{?rc_ver:~rc%{rc_ver}}
 

- Release:	2%{?dist}
 

+ Release:	3%{?dist}
 

  Summary:	The Low Level Virtual Machine
 

  

  License:	Apache-2.0 WITH LLVM-exception OR NCSA
 
@@ -90,6 +90,8 @@ 
 

  Source5:	https://github.com/llvm/llvm-project/releases/download/llvmorg-%{maj_ver}.%{min_ver}.%{patch_ver}%{?rc_ver:-rc%{rc_ver}}/%{third_party_srcdir}.tar.xz.sig
 

  Source6:	release-keys.asc
 

  

+ # Backported from LLVM 17
 

+ Patch1:		0001-SystemZ-Improve-error-messages-for-unsupported-reloc.patch
 

  # See https://reviews.llvm.org/D137890 for the next two patches
 

  Patch2:		0001-llvm-Add-install-targets-for-gtest.patch
 

  # RHEL-specific patch to avoid unwanted recommonmark dep
 
@@ -311,6 +313,7 @@ 
 

  %if %{without compat_build}
 

  	-DLLVM_VERSION_SUFFIX='' \
 

  %endif
 

+ 	-DLLVM_UNREACHABLE_OPTIMIZE:BOOL=OFF \
 

  	-DLLVM_BUILD_LLVM_DYLIB:BOOL=ON \
 

  	-DLLVM_LINK_LLVM_DYLIB:BOOL=ON \
 

  	-DLLVM_BUILD_EXTERNAL_COMPILER_RT:BOOL=ON \
 
@@ -577,6 +580,10 @@ 
 

  %endif
 

  

  %changelog
 

+ * Mon Jul 03 2023 Tulio Magno Quites Machado Filho <tuliom@redhat.com> - 16.0.6-3
 

+ - Improve error messages for unsupported relocs on s390x (rhbz#2216906)
 

+ - Disable LLVM_UNREACHABLE_OPTIMIZE
 

+ 

  * Wed Jun 14 2023 Tulio Magno Quites Machado Filho <tuliom@redhat.com> - 16.0.6-1
 

  - Update to LLVM 16.0.6

Backport a patch from LLVM 17 that improves error messages from the
Clang assembler on s390x.

Also, disable LLVM_UNREACHABLE_OPTIMIZE in order to improve error
messages when llvm_unreachable() is called. There are many cases where
llvm_unreachable() is used as a error reporting function instead of the
recommended replacement for assert().

Fixes rhbz#2216906.

rebased onto 7583740aaff6af57a96fe44302a4d2abccc2e985
2 months ago

rebased onto cd99de7
2 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/10309dbfa9cf45398fdf6ccfe58713de

LGTM

Also, disable LLVM_UNREACHABLE_OPTIMIZE in order to improve error
messages when llvm_unreachable() is called. There are many cases where
llvm_unreachable() is used as a error reporting function instead of the
recommended replacement for assert().

Not really. The error message passed to llvm_unreachable() will not be printed even if you disable LLVM_UNREACHABLE_OPTIMIZE. The only thing it does is make sure you get a trap at the position of the llvm_unreachable(), rather than falling through.

That said, we did test the impact of LLVM_UNREACHABLE_OPTIMIZE in Rust, and there was no observable impact on compile-time, so enabling the option is fine.

LGTM

Also, disable LLVM_UNREACHABLE_OPTIMIZE in order to improve error
messages when llvm_unreachable() is called. There are many cases where
llvm_unreachable() is used as a error reporting function instead of the
recommended replacement for assert().

Not really. The error message passed to llvm_unreachable() will not be printed even if you disable LLVM_UNREACHABLE_OPTIMIZE. The only thing it does is make sure you get a trap at the position of the llvm_unreachable(), rather than falling through.

Indeed! I didn't mean to imply that error messages would be printed.
But at least we do not have random failures because the compiler decided to optimize away the code that treated the error path. IMHO, that's an improvement, while it's still not ideal.

That said, we did test the impact of LLVM_UNREACHABLE_OPTIMIZE in Rust, and there was no observable impact on compile-time, so enabling the option is fine.

Thanks!

By the way, the tests failed because MLIR is still stuck at version 16.0.5 while LLVM source is already at 16.0.6.

Pull-Request has been merged by tuliom
2 months ago
Metadata
None
No Tags
Changes Summary 2
+184
file added
0001-SystemZ-Improve-error-messages-for-unsupported-reloc.patch
+8 -1
file changed
llvm.spec
Powered by Pagure 5.13.3
Documentation • File an Issue • About this Instance • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.