[PATCH] D38846: [ELF] - Linkerscript: fix issue with SUBALIGN.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/ScriptParser.cpp:649
+    if (!isPowerOf2_64(Alignment))
+      error(Loc + ": alignment must be power of 2");
+    return Alignment;
----------------
ruiu wrote:
> grimar wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > You should return 1 instead of some erroneous value.
> > > It does not make sence I think. 
> > > What we should do here is to report an error and a value that is not 0, so that possible alignTo() call will not asset.
> > > My code already do that.
> > "report an error and a value" -> "report an error and **return** a value"
> Well, it is actually "report an error and return a *sane* value". Alignment 17 is, for example, not a sane value, and your function shouldn't return such value even in an error condition. That is an important contract of your function, and it needs to satisfy that post-condition.
> 
> I believe you understand how lld handles error conditions very well, so it is a bit odd that you thought it doesn't make sense. It does make sense.
> 
> error() does not call exit(). You can call errors() as many times as you want to report multiple errors, and until the control reaches some checkpoint, lld continues working. While it is working, we need to maintain the integrity of our internal data structure so that, for example, lld wouldn't die with an assertion failure after reporting an error.
> 
> We do not expect Alignment to be a non-power-of-two value. So, you shouldn't return a non-power-of-two value from this function, even if there's an error in inputs. If you do, you are not only reporting an error but also propagating it to the caller and breaking our internal assumption.
Let me clarify. My point was that it should not make sence what to return here if such value allows to avoid assert/crash.
Currently the only place where we can assert I know about is a call of alignTo with zero, what is fixed by patch.
My code actually was heavily based on LLD's policy of handling error conditions - it did only thing that we had to do
to be able to exit on closest exit checkpoint after triggering a error.

It seems to me that if returning 17 can break something else (so that we assert/crash) because of our internal assumptions, it is at least a sign that we probably want to look closer at that place and probably may want to place one more exit checkpoint earlier. But I think I do not know such place currently. And most probably nothing too scary should happen until we reach existent exit checkpoint with alignment 17.

I am ok to return 1 here just in case for now to be consistent with internal assumption and to let this patch go though.


================
Comment at: test/ELF/linkerscript/subalign.s:26
+## Test we do not fail when dot(.) is used inside SUBALIGN. That is not
+## consistent with ld.bfd.
+# RUN: echo "SECTIONS { . = 0x32; .aaa : SUBALIGN(.) { *(.aaa*) } }" > %t3.script
----------------
jhenderson wrote:
> Could you comment here what we expect the behaviour to be in this case (apart from no error), please. For example, is the SUBALIGN value a) undefined, so may change, or b) always zero (effectively 1)? If it's undefined, I'm not sure we need the objdump check, since we may decide to change the behaviour later to something else, if it becomes more convenient.
I would say it is undefined. We do not want to support this behavior, it just works somehow now and that might change in future. Updated comment and testcase, thanks for looking !


https://reviews.llvm.org/D38846