ld seems to disregard section relative specifiers on symbols

[Julian Waters]

I'm happy to report back that not only does the @SECREL32 modifier
work as expected now:

# g++ tls.s

# ./a.exe
8

But it can also link the following program (Compiled by gcc with a TLS
enabling patch, which is not available to anyone yet):

thread_local int local = 2;
thread_local long long large = 7;

int main() {
    printf("%d\n", local);
    printf("%lld\n", large);
}

(Attached the resulting assembly from compiling the above in case
anyone wants it)
    .globl local
    .section .tls$,"w"
    .align 4
local:
    .long 2
    .globl large
    .align 8
large:
    .quad 7

    .seh_proc main
main:
.LFB0:
    pushq %rbp
    .seh_pushreg %rbp
    movq %rsp, %rbp
    .seh_setframe %rbp, 0
    subq $32, %rsp
    .seh_stackalloc 32
    .seh_endprologue
    call __main
    movl _tls_index(%rip), %eax
    movq %gs:88, %rdx
    movq (%rdx,%rax,8), %rax
    movl local@secrel32(%rax), %eax
    movl %eax, %edx
    leaq .LC0(%rip), %rax
    movq %rax, %rcx
    call printf
    movl _tls_index(%rip), %eax
    movq %gs:88, %rdx
    movq (%rdx,%rax,8), %rax
    movq large@secrel32(%rax), %rax
    movq %rax, %rdx
    leaq .LC1(%rip), %rax
    movq %rax, %rcx
    call printf
    movl $0, %eax
    addq $32, %rsp
    popq %rbp
    ret
    .seh_endproc

# ./a.exe
2
7

Seems like the patch was largely successful!

best regards,
Julian

On Mon, Sep 9, 2024 at 5:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.09.2024 14:58, Julian Waters wrote:
> > clang's lld correctly patches the code to use section relative offsets in
> > this instance, because the @secrel specifier was requested, while ld
> > incorrectly patches the address to be the absolute value. This appears to
> > be strictly an ld bug, because compiling with gcc or assembling with as and
> > then linking with clang lld yields the correct section relative offset of
> > 8. This bug is a significant showstopper in the recent attempts to add TLS
> > support on Windows to gcc, so I'm invested in having it fixed. Is there any
> > hint as to why ld is ignoring the @secrel modifier?
>
> Would you give the tentative (but not really complete) patch below a try,
> please? It being incomplete is not just for lacking a testcase, but:
>
> Mark, with this change one of the PDB tests yields "FAIL: Incorrect section
> contribution substream". In the absence of any documentation in ld-pe/pdb.exp,
> how is one to understand whether in fact something broke, or whether a test's
> expectations simply need updating (and then in which way)? I surely won't
> want to go from just what the new output is, but understand why it changed
> and why/whether it previously was correct in the first place. When putting
> together that test, it seems quite likely that you would have noticed that
> there's a bogus base relocation in the .exe, which apparently is related to
> what's being put in the .pdb.
>
> Jan
>
> ld/PE: no base relocs for section (relative) ones
>
> Even more so than image relative (RVA) relocations, section relative
> ones as well as section ones should not have base relocations created in
> the final PE image. Reportedly section relative relocations will want
> using for TLS support in the (Windows) compiler.
>
> While there also correct the names for two of the "image base" relocs.
> ---
> "Unused" (sometimes "absolute") relocations may similarly need special
> casing. Except for Arm (and i386 according to coff/i386.h, albeit other
> information I have - without a reference to where that once came from -
> says 0 has the same meaning there, and even 1 and 2 also have meaning),
> where no such type exists, that's always the relocation numbered 0. So a
> simple boolean may do there, unless the case is handled elsewhere.
>
> I'm surprised pe_detail_list[] has no IA64, LoongArch64, MCore, and
> RISC-V entries.
>
> --- a/ld/pe-dll.c
> +++ b/ld/pe-dll.c
> @@ -185,6 +185,9 @@ typedef struct
>    const char *target_name;
>    const char *object_target;
>    unsigned int imagebase_reloc;
> +  unsigned int secrel_reloc_lo;
> +  unsigned int secrel_reloc_hi;
> +  unsigned int section_reloc;
>    int pe_arch;
>    int bfd_arch;
>    bool underscored;
> @@ -257,11 +260,16 @@ static pe_details_type pe_detail_list[]
>  #ifdef pe_use_plus
>      "pei-x86-64",
>      "pe-x86-64",
> -    3 /* R_IMAGEBASE */,
> +    3 /* R_AMD64_IMAGEBASE */,
> +    11 /* R_AMD64_SECREL32 */,
> +    12 /* R_AMD64_SECREL7 */,
> +    10 /* R_AMD64_SECTION */,
>  #else
>      "pei-i386",
>      "pe-i386",
>      7 /* R_IMAGEBASE */,
> +    11, 11 /* R_SECREL32 */,
> +    10 /* R_SECTION */,
>  #endif
>      PE_ARCH_i386,
>      bfd_arch_i386,
> @@ -276,7 +284,10 @@ static pe_details_type pe_detail_list[]
>    {
>      "pei-x86-64",
>      "pe-bigobj-x86-64",
> -    3 /* R_IMAGEBASE */,
> +    3 /* R_AMD64_IMAGEBASE */,
> +    11 /* R_AMD64_SECREL32 */,
> +    12 /* R_AMD64_SECREL7 */,
> +    10 /* R_AMD64_SECTION */,
>      PE_ARCH_i386,
>      bfd_arch_i386,
>      false,
> @@ -287,6 +298,8 @@ static pe_details_type pe_detail_list[]
>      "pei-i386",
>      "pe-bigobj-i386",
>      7 /* R_IMAGEBASE */,
> +    11, 11 /* R_SECREL32 */,
> +    10 /* R_SECTION */,
>      PE_ARCH_i386,
>      bfd_arch_i386,
>      true,
> @@ -297,6 +310,7 @@ static pe_details_type pe_detail_list[]
>      "pei-shl",
>      "pe-shl",
>      16 /* R_SH_IMAGEBASE */,
> +    ~0, 0, ~0, /* none */
>      PE_ARCH_sh,
>      bfd_arch_sh,
>      true,
> @@ -306,6 +320,7 @@ static pe_details_type pe_detail_list[]
>      "pei-mips",
>      "pe-mips",
>      34 /* MIPS_R_RVA */,
> +    ~0, 0, ~0, /* none */
>      PE_ARCH_mips,
>      bfd_arch_mips,
>      false,
> @@ -315,6 +330,7 @@ static pe_details_type pe_detail_list[]
>      "pei-arm-little",
>      "pe-arm-little",
>      11 /* ARM_RVA32 */,
> +    ~0, 0, ~0, /* none */
>      PE_ARCH_arm,
>      bfd_arch_arm,
>      true,
> @@ -324,6 +340,8 @@ static pe_details_type pe_detail_list[]
>      "pei-arm-wince-little",
>      "pe-arm-wince-little",
>      2,  /* ARM_RVA32 on Windows CE, see bfd/coff-arm.c.  */
> +    15, 15, /* ARM_SECREL (dito) */
> +    14, /* ARM_SECTION (dito) */
>      PE_ARCH_arm_wince,
>      bfd_arch_arm,
>      false,
> @@ -332,13 +350,16 @@ static pe_details_type pe_detail_list[]
>    {
>      "pei-aarch64-little",
>      "pe-aarch64-little",
> -    2,  /* ARM64_RVA32 */
> +    2,  /* IMAGE_REL_ARM64_ADDR32NB */
> +    8,  /* IMAGE_REL_ARM64_SECREL */
> +    11, /* IMAGE_REL_ARM64_SECREL_LOW12L */
> +    13, /* IMAGE_REL_ARM64_SECTION */
>      PE_ARCH_aarch64,
>      bfd_arch_aarch64,
>      false,
>      autofilter_symbollist_generic
>    },
> -  { NULL, NULL, 0, 0, 0, false, NULL }
> +  { NULL, NULL, 0, 0, 0, 0, 0, 0, false, NULL }
>  };
>
>  static const pe_details_type *pe_details;
> @@ -1596,7 +1617,10 @@ generate_reloc (bfd *abfd, struct bfd_li
>                   printf ("rel: %s\n", sym->name);
>                 }
>               if (!relocs[i]->howto->pc_relative
> -                 && relocs[i]->howto->type != pe_details->imagebase_reloc)
> +                 && relocs[i]->howto->type != pe_details->imagebase_reloc
> +                 && (relocs[i]->howto->type < pe_details->secrel_reloc_lo
> +                     || relocs[i]->howto->type > pe_details->secrel_reloc_hi)
> +                 && relocs[i]->howto->type != pe_details->section_reloc)
>                 {
>                   struct bfd_symbol *sym = *relocs[i]->sym_ptr_ptr;
>                   const struct bfd_link_hash_entry *blhe
>
>