Differential D18190
[ELF] - Error out when R_X86_64_PC32/R_X86_64_32 are used against preemptible symbol when linking shared object. Authored by grimar on Mar 15 2016, 9:23 AM.
Details
gold errors out with something like "requires dynamic R_X86_64_32 reloc which Patch implements the same error.
Diff Detail
Event TimelineComment Actions It's going to print out the warning for each relocation. Isn't it going to be too many? Comment Actions 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). 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. Comment Actions 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? Comment Actions Should we care ? Its wrong by nature, do you want to replace error with fatal here ? Comment Actions 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.
Comment Actions
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. Comment Actions :) 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. Comment Actions From the source code Comment Actions 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. Comment Actions :) I would be happy not to do that. But that exact testcase use it (llvm33-rela-outside-group.s) : Input file generated with: Please look at r259831 commit text, it has explanations for the need of use of llvm 3.3 for that test. Comment Actions 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. Comment Actions 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 ? Comment Actions
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. Comment Actions 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. Comment Actions 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.
Comment Actions
I`ll update the patch with that change, George. Comment Actions Had to update this because new tests were failing:
Also included the next change:
Could this changes be reviewed again please ? | ||||||||||||||||||||||||||||||||||||||||