[PATCH] D30812: AsmPrinter: Don't treat symbols with prefix data as code

[Moritz Angermann via Phabricator via llvm-commits]

angerman planned changes to this revision.
angerman added a comment.

In https://reviews.llvm.org/D30812#698479, @pcc wrote:

> Does this really solve your problem? The linker should only create the trampoline if an object that takes a reference to the function with prefix data was built without `-fPIC`. If you change the symbol type to object I would expect the linker to create a copy relocation instead. That would cause the dynamic loader to create a copy of the function body in the main executable's address space, and that copy would not be in executable memory so you would not be able to call it.
>
> In fact, if I patch in your change and compile your test case, that is exactly what happens:
>
>   $ readelf -r main
>  
>   Relocation section '.rela.dyn' at offset 0x508 contains 2 entries:
>     Offset          Info           Type           Sym. Value    Sym. Name + Addend
>   000000600ff8  000400000006 R_X86_64_GLOB_DAT 0000000000000000 __gmon_start__ + 0
>   000000601040  000c00000005 R_X86_64_COPY     0000000000601040 hello + 0
>  
>   Relocation section '.rela.plt' at offset 0x538 contains 3 entries:
>     Offset          Info           Type           Sym. Value    Sym. Name + Addend
>   000000601018  000200000007 R_X86_64_JUMP_SLO 0000000000000000 printf + 0
>   000000601020  000300000007 R_X86_64_JUMP_SLO 0000000000000000 __libc_start_main + 0
>   000000601028  000400000007 R_X86_64_JUMP_SLO 0000000000000000 __gmon_start__ + 0
>
>
> and if I change your test case like this:
>
>   diff --git a/main.c b/main.c
>   index d54d1c5..accf187 100644
>   --- a/main.c
>   +++ b/main.c
>   @@ -5,5 +5,6 @@ int hello(void);
>    int main() {
>            int *prefix_data = (int*) &hello;
>            printf("hi: %d\n", prefix_data[-1]);
>   +        hello();
>            return 0;
>    }
>
>
> I get:
>
>   $ LD_LIBRARY_PATH=. ./main
>   hi: 0
>   Segmentation fault (core dumped)
>
>
> I think the correct fix is for you to arrange to build any objects that may access prefix data with `-fPIC` (or with the `-relocation-model=pic` flag to `llc`). If I change your test case to do that:
>
>   diff --git a/Makefile b/Makefile
>   index 84ccf76..9d22147 100644
>   --- a/Makefile
>   +++ b/Makefile
>   @@ -6,7 +6,7 @@ all : libtest.s main
>    libtest.so : libtest.o
>           gcc -shared -o $@ $+
>   
>   -main : main.o libtest.so
>   +main : main.c libtest.so
>           gcc -fPIC -L. -ltest -o $@ $+
>   
>    run : main
>
>
> the `main` binary works as intended.


Thank you @pcc for the thorough review and the lengthy test and explanation.  This
now ends up being a bit more puzzling that with what we started.  Especially as to
why the `function` to `object` rewrite we did, seemingly used to work. This should
not have worked, as you have demonstrated.

Our test case clearly fell prey to the implicit `%.o : %c` rule that make provides, and
which does not come with `-fPIC`.

I'll try to locate the issue we saw again and see where it deviates from the fixed test
case based on your suggestions to see if we still have a genuine issue or if there is/was
a bug in our toolchain use.

Thank you.


https://reviews.llvm.org/D30812