[PATCH] D38361: [ELF] Stop setting output section size early

[James Henderson via Phabricator via llvm-commits]

jhenderson updated this revision to Diff 126347.
jhenderson added a comment.

> @rafael wrote:
>  I don't like the idea of adding a member for the section position. At no point we need the position and the offset simultaneously and having both variables means they can get inconsistent.

As far as I'm aware there is no case where the two could get inconsistent, once set - that would require the ability to reorder the sections in the output section.

> One way to look at the current issue is that the early estimate of the offset is correct in the common case, which makes it really easy to use it incorrectly before the final value is computed.

I agree that using a value that is very different is likely to be spotted, although it's not guaranteed to be (especially if working with only a single InputSection in an output section).

> If we make the value almost always wrong, that problem goes away. What do you think of the attached patch? We could rename OutSecOff to OutSecOffsetOrIndex, but that doesn't seem worth it.

Given that both you and @ruiu seem reluctant to have new members, I've tentatively adopted it with a few changes and additions to the comments. I don't like using a member for two different purposes still, even if those two purposes don't overlap in time, but I'd rather get this change in in at least some form. Could you let me know what you think of the diff with the comment changes, please. In particular, I've moved the comments relating to the Size and OutSecOff members to make their dual purpose clear at their point of definition, and I've not added the suggested additional comments to maybeCompress - they're unnecessary as we already have a comment in assignOffsets saying that we don't support custom layout for compressed debug sections.

>> I'm wondering if it is feasible to check and disable compression if a non input section description command is detected in the debug section being compressed?
> 
> It should be possible to at least produce an error message.

I'd support something like that (it should be easy to add to the loop I'm adding in maybeCompress), but not as part of this change.


https://reviews.llvm.org/D38361

Files:
  ELF/InputSection.h
  ELF/LinkerScript.cpp
  ELF/OutputSections.cpp
  ELF/OutputSections.h
  test/ELF/linkerscript/unused-synthetic.s

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38361.126347.patch
Type: text/x-patch
Size: 4151 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171211/14be339c/attachment.bin>