Fix PR23871: Passing a string literal to .byte directive crashes the assembler

[enefaim]

Summary

Passing a string literal to .byte directive crashes LLVM when assembling for AArch64 target.

Output:
Unknown ELF relocation type
UNREACHABLE executed at /home/bgabor/work/clang-driver/byte-directive/llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:244!

This patch replaces the crash with an error message like the test-case in test/MC/ARM/Windows/invalid-relocation.s

[richard.barton.arm]

test/MC/AArch64/invalid-relocation.s

6

Wouldn't a nice error message be to output the string rather than an internal enum type?

[enefaim]

Hi Richard,

I agree that emitting source line information instead of the internal enum type would be
much better here. I just don't know how to get that information in the context of the code
in question.

[enefaim]

Hi Saleem,

I was told that you have a good understanding of this part of LLVM.
Could you take a look at this patch?

Thanks,
Gabor Ballabas

[rengolin]

Hi Gabor,

Can you explain the relation between the "a" parameter and the relocation FK_Dtata_1?

Can you correlate that enum type to the ARM ABI name?

Also, why do we need a special unsupported message for that relocation type only? The design of that switch clearly states that unsupported types are to break the compilation. If we start treating special relocation types with different behaviour, the code will become harder to maintain.

A better approach would be to either fail earlier (if that syntax is incorrect) in the assembler parser. Or, if the syntax *is* correct, to implement support for that relocation type (or use the correct relocation, if that's also wrong).

cheers,
--renato

[compnerd]

This really is the wrong way to handle this. .byte shouldn't accept the string literal in the first place. This should be diagnosed in the AsmParser. I can take a look at that if you like.

[enefaim]

Abandoning this revision because it has been fixed in another revision.
See details: https://llvm.org/bugs/show_bug.cgi?id=23871

lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp

          "Should only be expression-level modifiers here");
 
   assert((!Target.getSymB() ||
           Target.getSymB()->getKind() == MCSymbolRefExpr::VK_None) &&
          "Should only be expression-level modifiers here");
 
   if (IsPCRel) {
     switch ((unsigned)Fixup.getKind()) {
+    case FK_Data_1:
+      report_fatal_error("unsupported relocation type: FK_Data_1");
     case FK_Data_2:
       return ELF::R_AARCH64_PREL16;
     case FK_Data_4:
       return ELF::R_AARCH64_PREL32;
     case FK_Data_8:
       return ELF::R_AARCH64_PREL64;
     case AArch64::fixup_aarch64_pcrel_adr_imm21:
       assert(SymLoc == AArch64MCExpr::VK_NONE && "unexpected ADR relocation");

       return ELF::R_AARCH64_TSTBR14;
     case AArch64::fixup_aarch64_pcrel_branch19:
       return ELF::R_AARCH64_CONDBR19;
     default:
       llvm_unreachable("Unsupported pc-relative fixup kind");
     }
   } else {
     switch ((unsigned)Fixup.getKind()) {
+    case FK_Data_1:
+      report_fatal_error("unsupported relocation type: FK_Data_1");
     case FK_Data_2:
       return ELF::R_AARCH64_ABS16;
     case FK_Data_4:
       return ELF::R_AARCH64_ABS32;
     case FK_Data_8:
       return ELF::R_AARCH64_ABS64;
     case AArch64::fixup_aarch64_add_imm12:
       if (RefKind == AArch64MCExpr::VK_DTPREL_HI12)

test/MC/AArch64/invalid-relocation.s

+# RUN: not llvm-mc -triple aarch64-elf -filetype obj -o /dev/null 2>&1 %s \
+# RUN:     | FileCheck %s
+
+.byte("a")
+
+# CHECK: LLVM ERROR: unsupported relocation type: FK_Data_1