[PATCH] D52317: Add support for dynamic libraries on Hexagon

[Sid Manning via Phabricator via llvm-commits]

sidneym added inline comments.


================
Comment at: ELF/Arch/Hexagon.cpp:250-251
+      0x0e, 0xc0, 0x49, 0x6a, //   r14 = add (pc, ##GOTn at PCREL) }
+      0x1c, 0xc0, 0x8e, 0x91, //   r28 = memw (r14)
+      0x00, 0xc0, 0x9c, 0x52, //   jumpr r28
+  };
----------------
ruiu wrote:
> Indentation of the comment.
OK, <<2.


================
Comment at: test/ELF/hexagon-shared.s:5
+# RUN: ld.lld -shared %t2 -o %t3
+# RUN: ld.lld -shared %t %t3 -o %t4
+# RUN: llvm-objdump -d -j .plt %t4 | FileCheck %s
----------------
MaskRay wrote:
> MaskRay wrote:
> > This test may not be affected but some might (it happened in the past for a few times)
> > 
> > When a `t.so` is used to link `t`. `DT_SONAME` of `t.so` will end up in the `.dynstr` section of `t`. Its length may change the section size sometimes. FileCheck on `t` may get different results on some systems.
> > 
> > For intermediate `%t3` here, it'd be better to specify `-soname t3.so` to fix the length of its `DT_SONAME`.
> > DT_SONAME of t.so will end up in the .dynstr section of t
> 
> Correction: `DT_SONAME` (if exists) or filename
> 
> https://github.com/llvm-mirror/lld/tree/master/ELF/Driver.cpp#L232
> 
>     // DSOs usually have DT_SONAME tags in their ELF headers, and the
>     // sonames are used to identify DSOs. But if they are missing,
>     // they are identified by filenames. We don't know whether the new
>     // file has a DT_SONAME or not because we haven't parsed it yet.
>     // Here, we set the default soname for the file because we might
>     // need it later.
>     //
>     // If a file was specified by -lfoo, the directory part is not
>     // significant, as a user did not specify it. This behavior is
>     // compatible with GNU.
> 
> `%t` is a temporary filename including a directory name, it may be of varying lengths.
OK, I will commit using file extensions, .o and .so I will also add -soname to the link.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D52317