[PATCH] gold/aarch64: Fix adrp distance check

[PATCH] gold/aarch64: Fix adrp distance check

[Vladislav Khmelevsky]

The offset between destination and location is a signed number,
currently the offset is treated as unsigned number, thus mathematical
shifting of negative value is performed incorrectly.
---
 gold/aarch64.cc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index d2b0747ffdc..e3ab5f10faa 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -1182,7 +1182,9 @@ class Reloc_stub : public Stub_base<size, big_endian>
   aarch64_valid_for_adrp_p(AArch64_address location, AArch64_address dest)
   {
     typedef AArch64_relocate_functions<size, big_endian> Reloc;
-    int64_t adrp_imm = (Reloc::Page(dest) - Reloc::Page(location)) >> 12;
+    int64_t offset
+        = static_cast<int64_t> (Reloc::Page (dest) - Reloc::Page (location));
+    int64_t adrp_imm = offset < 0 ? ~(~offset >> 12) : offset >> 12;
     return adrp_imm >= MIN_ADRP_IMM && adrp_imm <= MAX_ADRP_IMM;
   }
 
-- 
2.25.1

Re: [PATCH] gold/aarch64: Fix adrp distance check

[Cary Coutant]

> The offset between destination and location is a signed number,
> currently the offset is treated as unsigned number, thus mathematical
> shifting of negative value is performed incorrectly.

Do you have a test case to illustrate the problem?

+    int64_t offset
+        = static_cast<int64_t> (Reloc::Page (dest) - Reloc::Page (location));
+    int64_t adrp_imm = offset < 0 ? ~(~offset >> 12) : offset >> 12;

C++ conventions: No space before paren in a function call.

The cast is unnecessary here for assignment to an int64_t. Once the
result is in a signed int, I don't think you need to go to all that
extra trouble to shift it.

I'd suggest this instead:

    int64_t adrp_imm = Reloc::Page(dest) - Reloc::Page(location);
    adrp_imm >>= 12;

-cary

Re: [PATCH] gold/aarch64: Fix adrp distance check

[Andreas Schwab]

On Jul 29 2022, Cary Coutant via Binutils wrote:

> The cast is unnecessary here for assignment to an int64_t. Once the
> result is in a signed int, I don't think you need to go to all that
> extra trouble to shift it.

Note that right shifting a negative value is fully defined only in C++20
and later.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] gold/aarch64: Fix adrp distance check

[Cary Coutant]

> > The cast is unnecessary here for assignment to an int64_t. Once the
> > result is in a signed int, I don't think you need to go to all that
> > extra trouble to shift it.
>
> Note that right shifting a negative value is fully defined only in C++20
> and later.

Yes, you're right, thanks. I was tempted to say that every compiler we
care about does that as an arithmetic shift, but of course that's
careless. Then I saw that the arm-gcc compiler translates that ?:
idiom into a single "asr" instruction. Kinda suggests we should have
an ASR macro somewhere common.

Still, the static cast is unnecessary, and I see no need to introduce
the temporary "offset" instead of "adrp_imm".

-cary

[PATCH] gold/aarch64: Fix adrp distance check

[Vladislav Khmelevsky]

Hello Cary, Andreas! Thank you for the review.

>> Do you have a test case to illustrate the problem?

Actually not, since we will need far call and add128+ mb binary to test that the gold properly calculates the distance. Or we might add union test, but AFAIK there are no such tests in binutils.

>> adrp_imm >>= 12;

As it was already said this is implementation defined, although usually it's true that it would result in arithmetic shift and would work.

As for the formatting I've add sign conversion since it might trigger the compiler warning, but if it is not needed I will remove it. As for the extra offset variable it looks equal to me so I don't have any problems with reformatting the patch as you've wrote :)

Thanks!
---
 gold/aarch64.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index d2b0747ffdc..514fad96789 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -1182,7 +1182,8 @@ class Reloc_stub : public Stub_base<size, big_endian>
   aarch64_valid_for_adrp_p(AArch64_address location, AArch64_address dest)
   {
     typedef AArch64_relocate_functions<size, big_endian> Reloc;
-    int64_t adrp_imm = (Reloc::Page(dest) - Reloc::Page(location)) >> 12;
+    int64_t adrp_imm = Reloc::Page (dest) - Reloc::Page (location);
+    adrp_imm = adrp_imm < 0 ? ~(~adrp_imm >> 12) : adrp_imm >> 12;
     return adrp_imm >= MIN_ADRP_IMM && adrp_imm <= MAX_ADRP_IMM;
   }
 
-- 
2.25.1

[PATCH] gold/aarch64: Fix adrp distance check

[Vladislav Khmelevsky]

The offset between destination and location is a signed number,
currently the offset is treated as unsigned number, thus mathematical
shift of a negative value is performed incorrectly.
---
 gold/aarch64.cc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index d2b0747ffdc..514fad96789 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -1182,7 +1182,8 @@ class Reloc_stub : public Stub_base<size, big_endian>
   aarch64_valid_for_adrp_p(AArch64_address location, AArch64_address dest)
   {
     typedef AArch64_relocate_functions<size, big_endian> Reloc;
-    int64_t adrp_imm = (Reloc::Page(dest) - Reloc::Page(location)) >> 12;
+    int64_t adrp_imm = Reloc::Page (dest) - Reloc::Page (location);
+    adrp_imm = adrp_imm < 0 ? ~(~adrp_imm >> 12) : adrp_imm >> 12;
     return adrp_imm >= MIN_ADRP_IMM && adrp_imm <= MAX_ADRP_IMM;
   }
 
-- 
2.25.1

Re: [PATCH] gold/aarch64: Fix adrp distance check

[Cary Coutant]

> The offset between destination and location is a signed number,
> currently the offset is treated as unsigned number, thus mathematical
> shift of a negative value is performed incorrectly.
> ---
>  gold/aarch64.cc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

This is OK. I've committed the patch on your behalf. Thanks!

-cary