[x265] PING [PATCH] aarch64: replace ldr pseudo-instruction with adrp+add

[Fangrui Song]

On Tue, Oct 18, 2022 at 5:30 AM chen <chenm003 at 163.com> wrote:
>
> Hi Song,
>
> This is a long time history bug. the '-DPIC' just add to compile option when Nasm is enabled, the bug can simple fix with below patch (NO TEST since I haven't ARM environment).
> After patch, the user may acivate these compile option with ENABLE_PIC=ON
>
> Could the x265 team please help verify patch?
>
> Regards,
> Min Chen
>
> diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt
> index 13e4750de..80f8e59a9 100755
> --- a/source/CMakeLists.txt
> +++ b/source/CMakeLists.txt
> @@ -266,6 +266,9 @@ if(GCC)
>              add_definitions(-DHAVE_NEON)
>          endif()
>      endif()
> +    if(ENABLE_PIC)
> +        list(APPEND ARM_ARGS -DPIC)
> +    endif()
>      add_definitions(${ARM_ARGS})
>      if(FPROFILE_GENERATE)
>          if(INTEL_CXX)
>

Hi Min, I think my patch as-is is correct. Whether aarch64 builds
wants -DPIC is a separate topic.
Is a maintainer able to verify and install this patch? Thanks

>  2022-10-18 14:37:10，"Fangrui Song" <maskray at google.com>
>
> On Thu, Oct 6, 2022 at 1:39 AM chen <chenm003 at 163.com> wrote:
>>
>> Hi Song,
>>
>>
>> I means the current tree support these adrp+add mode with compile option -DPIC, so we need not patch the code.
>>
>>
>> Regards,
>> Min Chen
>
>
> Hi Min, IIUC -DPIC is only defined for x86-64 (source/cmake/CMakeASM_NASMInformation.cmake).
> AArch64 does not get -DPIC.
>>
>> 2022-10-06 06:42:33，"Fangrui Song" <maskray at google.com>
>>
>> Hi Min, sorry but I just saw your question. I do not understand the request.
>> adrp+add is just strictly superior to ldr (which uses a constant pool and does not decrease code size) and avoids text relocations (which are prohibited in many systems).  `ldr             \rd, =\val+\offset` should just be removed. adrp+add also works on Mach-O systems as well.
>>
>>>
>>> At 2022-09-24 15:21:35, "Fangrui Song" <maskray at google.com> wrote:
>>> >Ping.  The breaks lld build and some binutils configurating defaulting
>>> >to disallow text relocations.
>>> >
>>> >On 2022-08-29, Fangrui Song wrote:
>>> >>On 2022-08-29, Fangrui Song wrote:
>>> >>>On 2022-08-30, chen wrote:
>>> >>>>Hi Song,
>>> >>>>
>>> >>>>
>>> >>>>Thank you for your patch.
>>> >>>>
>>> >>>>
>>> >>>>However, syntax of ':lo12:' depends on compiler, so more general LDR is better in here.
>>> >>>>
>>> >>>>
>>> >>>>Regards,
>>> >>>>Min Chen
>>> >>>
>>> >>>:lo12: is standard aarch64 assembly syntax.
>>> >>>Which aarch64 compiler supported by x265 does not support :lo12:?
>>> >>
>>> >>Note that LDR has another problem that it produced an absolute relocation R_AARCH64_ABS64. It will trigger an error
>>> >>when text relocations are disabled (default in ld.lld. See https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities#:~:text=Text%20relocations)
>>> >>
>>> >>```
>>> >>% : && /usr/bin/c++ -fPIC -O3 -DNDEBUG  -Wl,-Bsymbolic,-znoexecstack -shared ...
>>> >>ld: error: relocation R_AARCH64_ABS64 cannot be used against local
>>> >>symbol; recompile with -fPIC
>>> >>>>>defined in sad-a.S.o
>>> >>>>>>>> referenced by sad-a.S.o:(.text+0x9B00)
>>> >>```
>>> >>
>>> >>adrp+add work fine.
>>> >>
>>> >>(I am a maintainer of lld/ELF.)
>>> >>
>>> >>>>At 2022-08-30 02:33:37, "Fangrui Song" <maskray at google.com> wrote:
>>> >>>>>The ldr pseudo-instruction uses a literal pool, which is less efficient
>>> >>>>>and does not decrease the code size.
>>> >>>>>---
>>> >>>>>source/common/aarch64/asm.S | 4 +---
>>> >>>>>1 file changed, 1 insertion(+), 3 deletions(-)
>>> >>>>>
>>> >>>>>diff --git a/source/common/aarch64/asm.S b/source/common/aarch64/asm.S
>>> >>>>>index 399c37cf2..2506f50aa 100644
>>> >>>>>--- a/source/common/aarch64/asm.S
>>> >>>>>+++ b/source/common/aarch64/asm.S
>>> >>>>>@@ -130,11 +130,9 @@ ELF     .size   \name, . - \name
>>> >>>>>       adrp            \rd, \val+(\offset)
>>> >>>>>       add             \rd, \rd, :lo12:\val+(\offset)
>>> >>>>> .endif
>>> >>>>>-#elif defined(PIC)
>>> >>>>>+#else
>>> >>>>>       adrp            \rd, \val+(\offset)
>>> >>>>>       add             \rd, \rd, :lo12:\val+(\offset)
>>> >>>>>-#else
>>> >>>>>-        ldr             \rd, =\val+\offset
>>> >>>>>#endif
>>> >>>>>.endm
>>> >>>>>
>>> >>>>>--
>>> >>>>>2.37.2.672.g94769d06f0-goog
>>> >>>>>
>>> >>>>>_______________________________________________
>>> >>>>>x265-devel mailing list
>>> >>>>>x265-devel at videolan.org
>>> >>>>>https://mailman.videolan.org/listinfo/x265-devel
>>> >>>
>>> >>>>_______________________________________________
>>> >>>>x265-devel mailing list
>>> >>>>x265-devel at videolan.org
>>> >>>>https://mailman.videolan.org/listinfo/x265-devel
>>> >>>
>>> >_______________________________________________
>>> >x265-devel mailing list
>>> >x265-devel at videolan.org
>>> >https://mailman.videolan.org/listinfo/x265-devel
>>>
>>> _______________________________________________
>>> x265-devel mailing list
>>> x265-devel at videolan.org
>>> https://mailman.videolan.org/listinfo/x265-devel
>>
>>
>>
>> --
>> 宋方睿
>>
>> _______________________________________________
>> x265-devel mailing list
>> x265-devel at videolan.org
>> https://mailman.videolan.org/listinfo/x265-devel
>
>
>
> --
> 宋方睿
>
> _______________________________________________
> x265-devel mailing list
> x265-devel at videolan.org
> https://mailman.videolan.org/listinfo/x265-devel



-- 
宋方睿