[AVR] Use correct relocation for function pointers in globals

[aykevl]

Summary

Before this patch, function pointers would be emitted with the R_AVR_16
relocation. This relocation is correct for global addresses but not for
function addresses. Function pointers must use the relocation
R_AVR_16_PM.

This patch hopefully fixes this. I'm not entirely sure whether the patch
is entirely correct, but it does fix a number of cases (see the test
file).

[aykevl]

There is a test failure in llvm/test/MC/AVR/relocations-abs.s. With this change the LLVM behavior would start to differ from the avr-gcc behavior. But actually, the avr-gcc behavior might be a bug.

This is the affected code:

bar:
    .short 1f
1:

This is considered a function (not data) by the assembler, which you can see if you run nm:

$ avr-nm relocations-abs.o
00000000 t bar

I think all function pointers in globals should be using the R_AVR_16_PM relocation, no matter whether it's created in assembly or IR.

[aykevl]

• fixed MC test failure

[aykevl]

I could perhaps add a test to relocations-abs.s that .type foo,@object creates a relocation of type R_AVR_16?

[shepmaster]

llvm/lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp

74

Running this patch with my original code, it still doesn't work. If I print out symbol.getType(), it's 2, which seems to be STT_FUNC:

enum {
  STT_NOTYPE = 0,     // Symbol's type is not specified
  STT_OBJECT = 1,     // Symbol is a data object (variable, array, etc.)
  STT_FUNC = 2,       // Symbol is executable code (function, etc.)

I changed it to include both:

-      if (symbol.getType() == ELF::STT_NOTYPE)
+      auto type = symbol.getType();
+      if (type == ELF::STT_NOTYPE || type == ELF::STT_FUNC)

And this allowed my code to work.

[aykevl]

I experimented a bit more but I really can't find a way to tell the difference between undefined functions and undefined globals. Assuming one or the other will still be incorrect in some cases.

Maybe the situation can be improved from the current situation by only creating the R_AVR_16_PM relocation for defined functions? That part is easy, and fixes this bug in at least some cases (while not making it worse in other cases). Fixing this bug for undefined functions may require changes somewhere else in LLVM, and might be a bit too much for me.

[efriedma]

This patch doesn't look right. Maybe the following is helpful? https://godbolt.org/z/_JCX5d

[aykevl]

@efriedma yeah I know, it's not right in the current state. See my previous comment. Do you have any idea how it is possible to get the object type (function or data) when determining the relocation type, when the referenced symbol is external?

[efriedma]

In D82536#2124326, @aykevl wrote:

Do you have any idea how it is possible to get the object type (function or data) when determining the relocation type, when the referenced symbol is external?

The way avr-gcc works, the compiler is responsible for figuring this out, not the assembler. A code pointer should be emitted as .word gs(func) i.e. VK_AVR_GS.

[aykevl]

In D82536#2124348, @efriedma wrote:

The way avr-gcc works, the compiler is responsible for figuring this out, not the assembler. A code pointer should be emitted as .word gs(func) i.e. VK_AVR_GS.

Thank you for this!
In that case, I believe this patch is invalid and this feature should be implemented in a different way. However, I honestly have no idea where to even look to get this fixed.

I will abandon this now. The change may be useful for some people but can't be integrated into LLVM as-is.

[aykevl]

This patch has been superseded by D87631, which should actually be correct (unlike this patch).

llvm/lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp

case MCSymbolRefExpr::VK_None:
       return ELF::R_AVR_32;
     case MCSymbolRefExpr::VK_AVR_DIFF32:
       return ELF::R_AVR_DIFF32;
     }
   case FK_Data_2:
     switch (Modifier) {
     default:
       llvm_unreachable("Unsupported Modifier");
-    case MCSymbolRefExpr::VK_None:
+    case MCSymbolRefExpr::VK_None: {
+      auto &symbol = cast<MCSymbolELF>(Target.getSymA()->getSymbol());
+      if (symbol.getType() == ELF::STT_NOTYPE)
+        // Not a global variable, so probably a function pointer.
+        return ELF::R_AVR_16_PM;
       return ELF::R_AVR_16;
+    }
     case MCSymbolRefExpr::VK_AVR_NONE:
       return ELF::R_AVR_16_PM;
     case MCSymbolRefExpr::VK_AVR_DIFF16:
       return ELF::R_AVR_DIFF16;
     }
   case AVR::fixup_32:
     return ELF::R_AVR_32;
   case AVR::fixup_7_pcrel:

llvm/test/CodeGen/AVR/global-relocs.ll

+; RUN: llc < %s -march=avr -filetype=obj | llvm-objdump -Dr - | FileCheck %s
+
+; This test checks whether the correct relocation type (R_AVR_16 or
+; R_AVR_16_PM) is used for relocations in global variables (such as function
+; and global variable pointers). Function pointers need a special relocation
+; type so that the linker will use an address in program words (a pointer to
+; program space shifted left by one).
+
+target datalayout = "e-P1-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8"
+target triple = "avr"
+
+; Some globals to refer to.
+@someGlobal = dso_local constant i32 5, align 2
+@someProgmemGlobal = dso_local addrspace(1) constant i16 5, align 2
+
+; Reduced testcase from Rust.
+@MY_VTABLE = internal constant <{ i8 addrspace(1)* }> <{ i8 addrspace(1)* bitcast (void () addrspace(1)* @wake to i8 addrspace(1)*) }>, align 1
+; CHECK-LABEL: <MY_VTABLE>:
+; CHECK: R_AVR_16_PM wake
+
+@ptrToFunction = dso_local constant i8 addrspace(1)* bitcast (void () addrspace(1)* @wake to i8 addrspace(1)*), align 1
+; CHECK-LABEL: <ptrToFunction>:
+; CHECK: R_AVR_16_PM wake
+
+@ptrToGlobal = dso_local constant i32* @someGlobal, align 1
+; CHECK-LABEL: <ptrToGlobal>:
+; CHECK: R_AVR_16 someGlobal
+
+@ptrToProgmemGlobal = dso_local constant i16 addrspace(1)* @someProgmemGlobal, align 1
+; CHECK-LABEL: <ptrToProgmemGlobal>:
+; CHECK: R_AVR_16 someProgmemGlobal
+
+@ptrToProgmemGlobal2 = dso_local constant i8 addrspace(1)* bitcast (i16 addrspace(1)* @someProgmemGlobal to i8 addrspace(1)*), align 1
+; CHECK-LABEL: <ptrToProgmemGlobal2>:
+; CHECK: R_AVR_16 someProgmemGlobal
+
+@ptrToFunction2 = dso_local global void () addrspace(1)* @wake, align 1
+; CHECK-LABEL: <ptrToFunction2>:
+; CHECK: R_AVR_16_PM wake
+
+; A function to refer to.
+declare dso_local void @wake() addrspace(1)

llvm/test/MC/AVR/relocations-abs.s

 ; RUN: llvm-mc -filetype=obj -triple=avr %s | llvm-objdump -d -r - | FileCheck %s
 
+; WARNING: the behavior below differs from avr-gcc, avr-gcc will use R_AVR_16
+; instead of R_AVR_16_PM. I think this is a bug in avr-gcc.
+
 ; CHECK: <bar>:
 ; CHECK-NEXT: 00 00 nop
-; CHECK-NEXT: R_AVR_16 .text+0x2
+; CHECK-NEXT: R_AVR_16_PM .text+0x2
 bar:
     .short 1f
 1: