Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)

[Valery Ushakov <uwe%stderr.spb.ru@localhost>]

On Tue, Feb 16, 2021 at 18:51:43 -0500, Christos Zoulas wrote:

> On Feb 17,  2:20am, uwe%stderr.spb.ru@localhost (Valery Ushakov) wrote:
> -- Subject: Re: POINTER_ALIGNED_P (was: Re: CVS commit: src/sys)
> 
> | Well, it was you who did the definion of ALIGNED_POINTER centralized
> | and overridable :)
> | 
> |   revision 1.400
> |   date: 2012-01-25 00:03:36 +0400;  author: christos;  state: Exp;  lines: +26 -1;
> |   Use and define ALIGN() ALIGN_POINTER() and STACK_ALIGN() consistently,
> |   and avoid definining them in 10 different places if not needed.
> 
> If you look a bit deeper into that change, it merged many MD copies
> of this macro into one, and I needed the override for the archs that
> were different.
> 
> | ALIGNED_POINTER is overriden on x86 to be always true.  Surprisingly
> | it is not overriden for m68k and vax that are __NO_STRICT_ALIGNMENT.
> | That is most likely an oversight, but that will probably require some
> | cvs archaeology to confirm.  Some uses of ALIGNED_POINTER are inside
> | an __NO_STRICT_ALIGNMENT #ifdef.
> 
> This is a mess as you can see. The problem is that in each case we
> need to determine if this macro is used to test alignment to determine
> access restrictions on certain architectures (most cases), or it
> is done for performance/protocol requirements.
> 
> I hope that nothing falls into the second category and then we can
> use a single macro that uses a combination of __NO_STRICT_ALIGNMENT
> and __alignof() which my guess is that it did not exist at the time
> the macro was invented, and thus it used sizeof() and limited it to
> integral types.
> 
> | We don't even seem to be sure about its semantics, as far as I can
> | tell (see bus space comments in my mail).
> | 
> | That's even more of a reason to stop doing aimless random changes
> | without getting some kind of understanding first.  The last thing we
> | need is ALIGNED_POINTER and POINTER_ALIGNED macros with slighly
> | different semantics both of which are counter-intuitive to begin with
> | (and riastradh@ even had to add a verbose comment for that).
> 
> This change was not aimless. I wanted to remove the multiple copies of
> the m_copyup/m_pullup code into one function. To do that I needed the
> alignment as a value, not a predicate macro (that was again copied in
> ~10 places). When I tried to centralize it, I wanted to do the minimal
> change so I would not screw it up (and I did end up screwing it up).
> 
> This is a good opportunity now to clean this up even more and provide
> sane alignment macros.

We should start by at least separating the concerns.  The test for
"aligned at power of two boundary" and the actual intent/purpose of
that test ("stuff needs to be aligned for us to safely do $FOO").
Then we can test the alignment check without jumping through
ridiculous hoops.  That should have been done earlier for the
ALIGNED_POINTER change, but yeah, I can see how in the heat of the
moment that change was just "preserve the status quo" and that's
absolutely fine.  But doing the same thing now with the alignment test
and the purpose of that alignment test conflacted once again under a
different but confusignly similar name is negligent (if anything we
will run out of half way decent names for the just-the-alignment-test
macro, b/c all of them will be loaded with some additional accidental
semantic).


-uwe