[PATCH] D46653: Start support for linking object files with split stacks

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/InputSection.cpp:223
+  if (getFile<ELFT>() == nullptr)
+    return nullptr;
+
----------------
saugustine wrote:
> grimar wrote:
> > When can this happen? That code is perhaps dead.
> This function was refactored out of getLocation, and the check was added there for https://reviews.llvm.org/D30889.  getLocation still does the check.
> 
> Any synthetic section won't have a backing file. I have added a comment to that effect.
> 
> I don't think an assert would be any better for a client than returning a nullptr here, but I'll switch it if we want 100% code coverage. Up to you.
I do not think it is possible to call this function for synthetic sections for your case.

`getLocation` already have the check and your new code does not seem is able to call it for synthetics,
because I believe they have `Relocations` empty, so this piece is dead.
I would just remove it, assert would be excessive as code anyways would crash.


================
Comment at: ELF/InputSection.cpp:882
+    if (Rel.Sym->isLocal())
+      continue;
+    Defined *D = dyn_cast_or_null<Defined>(Rel.Sym);
----------------
saugustine wrote:
> grimar wrote:
> > This 'continue' is untested by a test case. The code is "dead" because of that.
> I've spent quite a bit of time trying to write a test case that triggers executes the continue, but I haven't managed to figure one out.
> 
> So I'm not sufficiently clever to write a test case to trigger this line, but I'm not convinced it can never happen.
The following test triggers this `continue`.

```
# REQUIRES: x86
# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
# RUN: ld.lld %t1.o -o %t

.local foo
foo:

.section .text,"ax", at progbits
 .quad foo
	
.section	.note.GNU-stack,"", at progbits
.section	.note.GNU-split-stack,"", at progbits
```

You could merge it with `x86-64-split-stack-prologue-adjust-success.s` I think.


================
Comment at: ELF/InputSection.cpp:887
+    if (!D)
+      continue;
+
----------------
saugustine wrote:
> grimar wrote:
> > This 'continue' is untested by a test case it seems.
> I've tried to write test-cases to execute this continue, but have been unable to do so. They protect against an erroneous or strange input.
> 
> I've just deleted these two lines, if someone manages to trigger this case, lld will just crash.
The lines are still there.

I was able to trigger this case after adding the `call foo at plt` and `-shared`
to your `x86-64-split-stack-prologue-adjust-success.s` test:


```
# RUN: ld.lld --defsym __morestack=0x100 --defsym __morestack_non_split=0x200 %t1.o %t2.o -shared -o %t
...

.size	split,. - split

call foo at plt
	
.section	.note.GNU-stack,"", at progbits
.section	.note.GNU-split-stack,"", at progbits
```


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D46653