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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/ScriptParser.cpp:649
+    if (!isPowerOf2_64(Alignment))
+      error(Loc + ": alignment must be power of 2");
+    return Alignment;
----------------
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.


https://reviews.llvm.org/D38846