Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/http_core.h include/httpd.h server/core.c

[Yann Ylavic]

On Fri, Feb 12, 2016 at 2:04 AM,   wrote:
> Author: wrowe
> Date: Fri Feb 12 01:04:58 2016
> New Revision: 1729929
>
[]
> Modified: httpd/httpd/trunk/include/httpd.h
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1729929=1729928=1729929=diff
> ==
> --- httpd/httpd/trunk/include/httpd.h (original)
> +++ httpd/httpd/trunk/include/httpd.h Fri Feb 12 01:04:58 2016
> @@ -1056,6 +1056,16 @@ struct request_rec {
>  apr_table_t *trailers_in;
>  /** MIME trailer environment from the response */
>  apr_table_t *trailers_out;
> +
> +/** Originator's DNS name, if known.  NULL if DNS hasn't been checked,
> + *  "" if it has and no address was found.  N.B. Only access this though
> + *  ap_get_useragent_host() */
> +char *useragent_host;

const char * ?
Christophe has made some changes recently to add const to r->protocol
(trunk only, can't be backported...), I think we shoudn't introduce
new char* in "public" structs.

> +/** have we done double-reverse DNS? -1 yes/failure, 0 not yet,
> + *  1 yes/success
> + *  TODO: 2 bit signed bitfield when this structure is compacted
> + */
> +int double_reverse;
>  };

I see this TODO about bitfield, but never seen it done in your
commits, so I wonder where Rainer's warning comes from...
Anyway, once backported (as you propose), we can hardly change it.

Regards,
Yann.

Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/http_core.h include/httpd.h server/core.c

[William A Rowe Jr]

On Feb 12, 2016 2:24 AM, "Yann Ylavic"  wrote:
>
> On Fri, Feb 12, 2016 at 2:04 AM,   wrote:
> > Author: wrowe
> > Date: Fri Feb 12 01:04:58 2016
> > New Revision: 1729929
> >

> > +/** Originator's DNS name, if known.  NULL if DNS hasn't been
checked,
> > + *  "" if it has and no address was found.  N.B. Only access this
though
> > + *  ap_get_useragent_host() */
> > +char *useragent_host;
>
> const char * ?
> Christophe has made some changes recently to add const to r->protocol
> (trunk only, can't be backported...), I think we shoudn't introduce
> new char* in "public" structs.

While I would generally agree, the note states you must not access this
member directly, so it seems irrelevant.  But such a change should be
harmless.

> > +/** have we done double-reverse DNS? -1 yes/failure, 0 not yet,
> > + *  1 yes/success
> > + *  TODO: 2 bit signed bitfield when this structure is compacted
> > + */
> > +int double_reverse;
> >  };
>
> I see this TODO about bitfield, but never seen it done in your
> commits, so I wonder where Rainer's warning comes from...
> Anyway, once backported (as you propose), we can hardly change it.

Precisely, it will NOT change in 2.4 and should not change, and is only
applicable to trunk.  I should have omitted from the initial commit.

If we define this as int :2 and add a second int :2, none of the c
standards promise that the initial 2-bit value maintains its previous
alignment.  The alignment is undefined.

When we refactor trunk structures in anticipation of 2.6/3.0, that will be
the time to permanently park short bit values adjacent to one another.

If backported, we should redact the comment.

Cheers,

Bill

Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/http_core.h include/httpd.h server/core.c

[Yann Ylavic]

On Fri, Feb 12, 2016 at 10:49 AM, William A Rowe Jr  wrote:
> On Feb 12, 2016 2:24 AM, "Yann Ylavic"  wrote:
>>
>> I see this TODO about bitfield, but never seen it done in your
>> commits, so I wonder where Rainer's warning comes from...
>> Anyway, once backported (as you propose), we can hardly change it.
>
> Precisely, it will NOT change in 2.4 and should not change, and is only
> applicable to trunk.  I should have omitted from the initial commit.
>
> If we define this as int :2 and add a second int :2, none of the c standards
> promise that the initial 2-bit value maintains its previous alignment.  The
> alignment is undefined.

Indeed, and as such we can't make any promise about bits
alignment/order wrt to the underlying int, and the user can't expect
any either.

As you noticed, it's not possible to get the address of a bitfield (it
makes no sense), and we don't union the bitfields with a true int, so
the user would have to do things like "int *bitmask = (int *)&((char
*)r)[APR_OFFSETOF(r, useragent_host) + sizeof(r->useragent_host)]" to
start playing with the bits, and IMO that deserves a breakage if we
add a new bitfield!

So I don't thing we should worry about compatibility of bitfields wrt
added bits, the goal is to be able to add a new field without
increasing the size of the struct (for something that requires only a
few bits).

Aside note: with an int:2 bitfield, the possible values are 0 (00), 1
(01), -2 (10) and -1 (11), that may be surprising, although that fits
your description of double_reverse.
IMHO signed bitfields should be avoided, and I don't know if compilers
are kind enough (nor allowed) to pack successive signed/unsigned
bitfields, probably yes but otherwise we may lose extensibility of
"int double_reverse:2" though...
So we may prefer "unsigned int double_reverse:2" (and change the
description) so that we can later add a single bit that equals 1 and
not -1.

Regards,
Yann.

Re: svn commit: r1729929 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/http_core.h include/httpd.h server/core.c

[William A Rowe Jr]

On Feb 12, 2016 04:45, "Yann Ylavic"  wrote:
>
> On Fri, Feb 12, 2016 at 10:49 AM, William A Rowe Jr 
wrote:
> > On Feb 12, 2016 2:24 AM, "Yann Ylavic"  wrote:
> >>
> >> I see this TODO about bitfield, but never seen it done in your
> >> commits, so I wonder where Rainer's warning comes from...
> >> Anyway, once backported (as you propose), we can hardly change it.

Well we simply can't reorder bitfields or add them in 2.4 anyways...

> > Precisely, it will NOT change in 2.4 and should not change, and is only
> > applicable to trunk.  I should have omitted from the initial commit.
> >
> > If we define this as int :2 and add a second int :2, none of the c
standards
> > promise that the initial 2-bit value maintains its previous alignment.
The
> > alignment is undefined.
>
> Indeed, and as such we can't make any promise about bits
> alignment/order wrt to the underlying int, and the user can't expect
> any either.

Therefore this is problematic for 2.4, and I added a comment to STATUS that
we drop that note if backporting the patch.  The note applies only to trunk.

> So I don't thing we should worry about compatibility of bitfields wrt
> added bits, the goal is to be able to add a new field without
> increasing the size of the struct (for something that requires only a
> few bits).

And that is the noble goal heading toward 2.next (3.0?) but it seems there
is no promise that adding one int foo:2 value following a previously
released int bar:2 member would preserve the bit alignment of bar across
all compilers?  That suggests possibly breaking our ABI contract on a
maintenance release.  So adding discrete bit fields seem very iffy, once we
tag a specific release branch.  (I can see safe exceptions if adding a
group of small ints together in a maintenance release, but you would want
to bookend that with a member of known alignment, an int or * or such.)

> Aside note: with an int:2 bitfield, the possible values are 0 (00), 1
> (01), -2 (10) and -1 (11), that may be surprising, although that fits
> your description of double_reverse.

You are only looking at 2's compliment architectures, httpd isn't Intel
specific :)  But we are promised 1..-1, while the fourth value is -2, or
-0, or 2, or NaN.  Not something to rely on, but we don't need a fourth
value here anyways.

> IMHO signed bitfields should be avoided, and I don't know if compilers
> are kind enough (nor allowed) to pack successive signed/unsigned
> bitfields, probably yes but otherwise we may lose extensibility of
> "int double_reverse:2" though...
> So we may prefer "unsigned int double_reverse:2" (and change the
> description) so that we can later add a single bit that equals 1 and
> not -1.

We've had good success, they get rearranged and optimized only when it is
convenient for us to break ABI.

Unsigned bitfields are sometimes more compact and get us one more value (we
cannot reliably use a -2 value in a 2 bit int as I mentioned above) but
this particular use doesn't seem problematic to optimize later (in
trunk)... Let's leave it as a full int to avoid future ABI alignment issues
for any 2.4 backport?