Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

fix the detail for PC-relative relocations and add explanation #58

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xry111
Copy link
Contributor

@xry111 xry111 commented Jul 25, 2022

In the BFD code (under review now), we have:

#define RELOCATE_CALC_PC32_HI20(relocation, pc) 	\
  ({						\
    bfd_vma lo = (relocation) & ((bfd_vma)0xfff);	\
    pc = pc & (~(bfd_vma)0xfff);			\
    if (lo > 0x7ff)					\
      {						\
        relocation += 0x1000;			\
      } 						\
    relocation &= ~(bfd_vma)0xfff;			\
    relocation -= pc;				\
  })

#define RELOCATE_CALC_PC64_HI32(relocation, pc)  	\
  ({						\
    bfd_vma lo = (relocation) & ((bfd_vma)0xfff);	\
    if (lo > 0x7ff)					\
      { 						\
        relocation -= 0x100000000;      		\
      }  						\
    relocation -= (pc & ~(bfd_vma)0xffffffff);  	\
  })

Pay attention at relocation += 0x1000 and relocation -= 0x100000000.

Currently our detailed description does not strictly match it.
This will puzzle the engineer implementing or reviewing the relocs for
other linkers (gold/lld/mold) and cause we criticized by the upstream
reviewers. Fix the pseudo-code in the "detail" column and add two
footnotes as explanation about why the additional operation is needed.

In the BFD code (under review now), we have:

    #define RELOCATE_CALC_PC32_HI20(relocation, pc) 	\
      ({						\
        bfd_vma lo = (relocation) & ((bfd_vma)0xfff);	\
        pc = pc & (~(bfd_vma)0xfff);			\
        if (lo > 0x7ff)					\
          {						\
            relocation += 0x1000;			\
          } 						\
        relocation &= ~(bfd_vma)0xfff;			\
        relocation -= pc;				\
      })

    #define RELOCATE_CALC_PC64_HI32(relocation, pc)  	\
      ({						\
        bfd_vma lo = (relocation) & ((bfd_vma)0xfff);	\
        if (lo > 0x7ff)					\
          { 						\
            relocation -= 0x100000000;      		\
          }  						\
        relocation -= (pc & ~(bfd_vma)0xffffffff);  	\
      })

Pay attention at `relocation += 0x1000` and `relocation -= 0x100000000`.

Currently our detailed description does not strictly match it.
This will puzzle the engineer implementing or reviewing the relocs for
other linkers (gold/lld/mold) and cause we criticized by the upstream
reviewers.  Fix the pseudo-code in the "detail" column and add two
footnotes as explanation about why the additional operation is needed.
Copy link
Contributor

@xen0n xen0n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

认同。我自己实现 LLD 时候很是烧了一些脑细胞。。

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants