[llvm] r323297 - Don't assume a null GV is local for ELF and MachO.

[Rafael Avila de Espindola via llvm-commits]

First, sorry for the delay in replying.

Brooks Moses <bmoses at google.com> writes:

> The patch also affects the generated assembly code, which with the
> non-integrated assembler also causes changes in the undefined symbols in
> the object files.  Since grub also does pre-processing of the object files
> to figure out dependencies, that gets broken as well.
>
> Suppose we have a very basic C program that calls memset:
>
> $ cat foo.c
> void *memset (void *s, int c, unsigned int n);
> void foo(char *c, int len) {
>   memset(c, 32, len);
> }
>
> If we compile this with -m32, -S and some level of optimization (not -O0),
> we get a call to "memset at PLT" in the assembly file (where previously we
> just got "memset"):
> $ clang -m32 -O3 -S -o foo.s foo.c; grep memset foo.s
>         calll   memset at PLT
>
> When I then build an object file using -no-integrated-as (and binutils
> 2.24), we get an extra undefined reference to _GLOBAL_OFFSET_TABLE_:
> $ clang -m32 -O3 -no-integrated-as -c -o foo.o foo.c; nm foo.o
> 00000000 T foo
>          U _GLOBAL_OFFSET_TABLE_
>          U memset

Yes, that is an unfortunate case where gas leaks what should be an
implementation detail. This is so common that anything analyzing symbols
should be prepared to handle it. The linux kernel has that in
scripts/mod/modpost.c for example.

> So that's the first problem.  I worked around that by editing grub's
> dependency analyzer to just ignore the symbol,

I think that is the right fix and there is precedent for it.

> and ended up with another
> problem: Grub's linker now exits with "unsupported relocation 0x4" in this
> code block:
> https://github.com/coreos/grub/blob/d3fd939f18446b05d1d5456f23823498a1eb3fb4/util/grub-module-verifierXX.c#L277
>
> I don't think that issue is nearly as amenable to simple workarounds -- and
> even if it is, requiring patches to grub in order to support the latest
> LLVM version seems like a problem.

I actually think it should be fairly easy to fix. Do you have build
instructions handy?

>From llvm's point of view I would say that it is all missing features in
grubs partial implementation of the ABI.

I am happy helping implement the missing bits in grub, but I don't think
we should avoid this change in llvm.

> Also, for what it's worth, while I was investigating this I became doubtful
> of the claim in the commit message that this "should help with avoiding a
> plt reference when calling an intrinsic with -fno-plt".  Adding -fno-plt to
> my test compilations still results in having a "calll memset at PLT" in the
> generated assembly where we previously had "calll memset", which seems
> directly counter to that.

That was a reference to a change that is still in code review.

Cheers,
Rafael