Re: [PATCH] [4.5 regression] C++ ignores some aligned attributes

["Ulrich Weigand" <uweigand@de.ibm.com>]

Paul Brook wrote:
> > Well, the only thing my patch does is to revert behavior back to what it
> > was in 4.4 and earlier, after having been (clearly inadvertently) changed
> > by an unrelated patch.  At least Firefox relies on that behavior; Firefox
> > builds would certainly break if we add the error you suggest ...
> 
> I'd argue that this is a feature.  Code already broke (PR45112) once due to 
> mismatched alignment assumptions, so who knows what other bugs are lurking 
> behind this mismatch.  Better to fail at compile time than runtime.

With this patch to fix PR45112 (which restores prior behavior), there *is*
no bug lurking in Firefox; the Firefox code seems perfectly fine to me,
and I see no reason to deliberately break Firefox builds here.

PR45112 did not break because of mismatched alignment attributes; rather
it broke because the compiler *ignored* the (one) alignment attribute on the
definition, due to an unintended effect of the patch moving bits around.

There is no code in Firefox that relies on having an alignment attribute
visible on the declaration.  There certainly is the (implicit) assumption
that all JSVAL pointers must be 8 byte aligned, but this is not really
up to the compiler to enforce (most such pointers come from malloc anyway);
it is a requirement upon users of that interface.  (Which they tried to
satisfy using an aligned attibute -- which was then ignored due to the
GCC bug.)

> It seems rather inconsistent to require any particular behavior in the 
> presence of this inconsistency, but not provide a diagnostic for all the other 
> closely related cases that we know are still going to break. If nothing else 
> this seems like a missed optimization opportunity.

I'm not sure I agree that there is an inconsistency.  In general, in C
(or C++), declarations need not necessarily agree on each property they
specify; for example, you can have a declaration with "static" and one
without "static" for the same function.  These properties are then merged.
I do not see why aligned attributes should necessarily be different here;
introducing this as requirement seems to be a change with the potential to
break existing code bases that otherwise wouldn't have any problems.

Now, this property does of course give rise to the possibility that it
can be used in incorrect ways that introduce bugs.  But I don't agree
that the Firefox usage is of that type.  It simply requests that the
variable is actually allocated using stricter alignment requirements
than would otherwise be the case.  (Note that you could have the same
effect by specifying that extra alignment in a linker script!)


I guess I could see an argument to be made for a diagnostic if the alignment
at the definition is *less* strict than the one at some other declaration,
and we know about it at the point.  This diagnostic in fact would not break
Firefox either, because that's not the case there.

However, as Mark mentioned, that diagnostic wouldn't be a sure way to
recognize all instances, because that other declaration might not
even be visible at the site of the definition ...


[ In either case, that seems a completely separate argument from whether
the original patch in this thread ought to be applied -- which it of
course already is.  In my mind that patch fixes a wrong-code regression
by restoring prior behaviour.  If we decide that additional diagnostics
are useful in certain cases, that would be an enhancement on top.  ]

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com