[PATCH] D55550: [LLD][ELF] - Fix the different behavior of the linker script symbols on different platforms.

Wed Apr 17 23:51:40 PDT 2019

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM

Changes to the linker script processor oftentimes has subtle implications and in order to review such change I have to stop and think. But for this patch I took too much time. Apologies for the delay.

> Somewhat off-topic; I'm wondering if it is worth revisiting this in a wider context to see if we can find a better way of resolving these compatibility problems. At EuroLLVM there was an interesting discussion in the binutils BOF/Round-table on how important compatibility with GNU tools was. The general consensus was that it was particularly important in projects that had to support both sets of tools and that rewriting inputs to work around limitations in one or other of the tools was a significant headache. The overall philosophy to be taken forward was for GNU compatible option names and output wherever possible, i.e. unless the GNU behaviour was impossible to implement or was crazy anyway. I think that it would be a reasonable to come up with a similar position for LLD, perhaps an RFC to LLVM-DEV might be a way forward.

I think that's a fair argument and we should take that stance; as long as it's not impossible to implement nor too crazy, we should make lld compatible with GNU bfd linker whenever possible at least in terms of linker script compatibility. I don't personally like the linker script and perhaps the ELF file format itself (I dislike the fact that segments and sections are orthogonal in the ELF format), but in many cases a linker script just does its job, and without that some kind of task is not doable. In practice, GNU compatibility is very important.

One of the reasons why it is not easy to review linker script code change is because it's complicated. I don't feel I fully understand the existing code of the linker script processor.  It grows organically, and in many cases a small issue can be resolved by adding another call of assignOffsets(). It would for sure fixes a problem, but it is sometimes hard to say whether that's correct or not. Each stage doesn't seem clearly separated, and it is sometimes not clear whether doing something in some stage is the right thing or not. Is this due to the nature of the complexity of the linker script? I don't think so -- we didn't understand the linker script well when we started implementing it a few years ago, and the way how we organized code in the few years was still there. But now we understand it well including corner cases. I want to sit down and think hard about how to (re)organize the code. However, that idea seems the cause of slow review of linker script patches (I always started thinking how this should really be organized). I'll defer the idea of refactoring and let me review linker script changes more casually. I'm sorry about the delay, again.

================
Comment at: ELF/Writer.cpp:1503

   for (;;) {
     bool Changed = false;
----------------
Can you mention that for x86 this loop iterates only once?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55550/new/

https://reviews.llvm.org/D55550