This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Error out when R_X86_64_PC32/R_X86_64_32 are used against preemptible symbol when linking shared object.
ClosedPublic

Authored by grimar on Mar 15 2016, 9:23 AM.

Details

Summary

gold errors out with something like "requires dynamic R_X86_64_32 reloc which
may overflow at runtime; recompile with -fPIC" when this relocations are
used against preemptible symbol and output is position independent.

Patch implements the same error.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 50746.Mar 15 2016, 9:23 AM
grimar retitled this revision from to [ELF] - Error out when R_X86_64_PC32/R_X86_64_32 are used against preemptible symbol when linking shared object..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
ruiu edited edge metadata.Mar 15 2016, 11:01 AM

It's going to print out the warning for each relocation. Isn't it going to be too many?

emaste added a subscriber: emaste.Mar 15 2016, 11:15 AM
In D18190#375694, @ruiu wrote:

It's going to print out the warning for each relocation. Isn't it going to be too many?

It's going to error out and stop linking. For R_X86_64_PC32 and R_X86_64_32 relocations agains preemptible symbols when linking DSO (PIC actually, for -pie it should be the same, but patch about -pie is not yet committed).
That is how execution reaches here from:

if (Preemptible) {
  // We don't know anything about the finaly symbol. Just ask the dynamic
  // linker to handle the relocation for us.
  Out<ELFT>::RelaDyn->addReloc({Target->getDynRel(Type), &C, RI.r_offset,
                                false, &Body, getAddend<ELFT>(RI)});
  continue;
}

I believe we should do that check too. At least as you can see - many of our tests were affected.

grimar updated this revision to Diff 50803.Mar 16 2016, 2:02 AM
grimar edited edge metadata.
  • Corrected broken test
  • Rebased
ruiu added a comment.Mar 16 2016, 6:50 AM

So, if an object file has a PC32 relocation against that needs a dynamic relocation, then it also has a lot of PC32 relocation of the same kind, I guess?

Doesn't that mean when the user see this "recompile with -pic" error message, the error message is printed out, say, 100 times when you run the linker?

In D18190#376341, @ruiu wrote:

So, if an object file has a PC32 relocation against that needs a dynamic relocation, then it also has a lot of PC32 relocation of the same kind, I guess?

Doesn't that mean when the user see this "recompile with -pic" error message, the error message is printed out, say, 100 times when you run the linker?

Should we care ? Its wrong by nature, do you want to replace error with fatal here ?

ruiu added a comment.Mar 16 2016, 7:24 AM

Users may care, but it is hypothesis, so let's not take care of that for now. We may want to de-dup warnings in future.

test/ELF/Inputs/llvm33-rela-outside-group.o
1–2 ↗(On Diff #50803)

Can you use llmv-as to generate this file?

grimar added inline comments.Mar 16 2016, 7:29 AM
test/ELF/Inputs/llvm33-rela-outside-group.o
1–2 ↗(On Diff #50803)

Could you please clarify ? I downloaded and used llvm 3.3 binaries for that. Do you think I can generate this using only command line of modern llvm-as ? I really doubt in that.

In D18190#376380, @ruiu wrote:

Users may care, but it is hypothesis, so let's not take care of that for now. We may want to de-dup warnings in future.

It is not warning, that is an error..

ruiu added a comment.Mar 16 2016, 7:32 AM

Sorry, I meant llvm-mc.

This is not a fatal error which immediately let the linker terminate at least. So from the user's point of view, our error sometimes looks like a warning that just prints out a lot of > error messages (although it will eventually make the entire link fail at some point in future.)

That is an issue I believe. We probably want to fatal() at some point, say 50 errors for example.

(That contradicts? using lld as a lib I think)

George.

In D18190#376390, @ruiu wrote:

Sorry, I meant llvm-mc.

:) Ok. I mean llvm 3.3 llvm-mc was used originally to generate this. And my patch changed assembly but still requires llvm 3.3 llvm-mc to generate output.
And that what I used.
So I think we still need to have .o file here.

ruiu added a subscriber: ruiu.Mar 16 2016, 7:59 AM

From the source code
https://llvm.org/svn/llvm-project/llvm/trunk/tools/llvm-mc/llvm-mc.cpp, it
looks like it has the -relocation-model flag.

In D18190#376414, @ruiu wrote:

From the source code
https://llvm.org/svn/llvm-project/llvm/trunk/tools/llvm-mc/llvm-mc.cpp, it
looks like it has the -relocation-model flag.

I am not sure what you mean here. That test was created because it used something that llvm 3.3 llvm-mc generated. We still need to use 3.3 then here, doesn't we ? No matter what command line it has.

ruiu added a comment.Mar 16 2016, 8:05 AM

Why do you have to use 3.3?

In D18190#376416, @ruiu wrote:

Why do you have to use 3.3?

:) I would be happy not to do that. But that exact testcase use it (llvm33-rela-outside-group.s) :

Input file generated with:
llvm33/llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %S/Inputs/llvm33-rela-outside-group.o

Please look at r259831 commit text, it has explanations for the need of use of llvm 3.3 for that test.

ruiu added a comment.Mar 16 2016, 8:29 AM

Ah, gotcha. So this change would break the existing test for r259831 because it would now fail because of "recompile with -fPIC" error, so you had to update it.

But you don't have to update the binary. Remove "-shared" from llvm33-rela-outside-group.s.

In D18190#376427, @ruiu wrote:

Ah, gotcha. So this change would break the existing test for r259831 because it would now fail because of "recompile with -fPIC" error, so you had to update it.

But you don't have to update the binary. Remove "-shared" from llvm33-rela-outside-group.s.

I was not familar with that test and commit. If you think we can just remove -shared, lets do that separately, before checking in this one ?

If you are not familiar with that test, then please take time to understand it. You were about to modify the input to that particular test. If you don't understand what you > are doing, you may be changing the test in such a way that it would make no longer sense.

I modified the test in part of my needs. That change seemed and seems obvious for me. I agree with your position though, let return back to this talk tomorrow after I`ll investigate this testcase more deeply.

George.

In D18190#376427, @ruiu wrote:

Ah, gotcha. So this change would break the existing test for r259831 because it would now fail because of "recompile with -fPIC" error, so you had to update it.

But you don't have to update the binary. Remove "-shared" from llvm33-rela-outside-group.s.

So after investigation and looking on Sean's comment I think my solution was OK. My solution avoids useless stuff like add of "_start" and so on. So I agree with Sean here.
At the same time you was right when saying that I was not convinced about this test changes. Now I am.

So what about this one ?

rafael accepted this revision.Mar 28 2016, 6:09 AM
rafael edited edge metadata.

LGTM with nits.

I think this is OK.

Once we have a better foundation we should spend time trying to get better error messages, but for now this at least makes sure we notice the error.

ELF/Target.cpp
739 ↗(On Diff #50803)

I am not a very big fan of the wording.

The relocation *can* be used when making a shared object, it just has to resolve.

We could say something like "cannot be used in a dynamic relocation", but not sure if that is more useful.

test/ELF/x86-64-reloc-32-fpic.s
6 ↗(On Diff #50803)

You don't need _start in here.

test/ELF/x86-64-reloc-pc32-fpic.s
6 ↗(On Diff #50803)

or here.

This revision is now accepted and ready to land.Mar 28 2016, 6:09 AM

LGTM with nits.

I think this is OK.

Once we have a better foundation we should spend time trying to get better error messages, but for now this at least makes sure we notice the error.

Thanks for review !

grimar added inline comments.Mar 28 2016, 6:58 AM
ELF/Target.cpp
739 ↗(On Diff #50803)

We probably can change this to gold version: "requires dynamic R_X86_64_32 reloc which may overflow at runtime; recompile with -fPIC".
At least that is consistent.

How about "R_X86_64_32 cannot be a dynamic relocation"?

In any case. We can word smith it after commit.

Cheers,
Rafael

I`ll update the patch with that change,
because observing 2 new failing tests that going to fix as well.

George.

grimar updated this revision to Diff 51799.Mar 28 2016, 9:54 AM
grimar edited edge metadata.

Had to update this because new tests were failing:

  • Updated new /ELF/lto/linkonce-odr.ll and /ELF/lto/linkonce.ll tests to pass after this path changes.

Also included the next change:

  • Changed text to "R_X86_64_32 cannot be a dynamic relocation"

Could this changes be reviewed again please ?

grimar requested a review of this revision.Mar 28 2016, 11:28 AM
grimar edited edge metadata.
rafael accepted this revision.Mar 28 2016, 3:20 PM
rafael edited edge metadata.

LGTM.

Thanks

This revision is now accepted and ready to land.Mar 28 2016, 3:20 PM
This revision was automatically updated to reflect the committed changes.