How to align shm in an neat way?

[Rainer Jung <ra...@kippdata.de>]

Hi,

PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One 
of the three structs mapped into shm contains an apr_time_t member, 
which at least on Sparc is 8 Bytes, whereas for 32 bit builds long is 
only 4 Bytes.

Currently everything is aligned for 4 Bytes, so we get bus 
errors/crashes when trying to assign the apr_time_t to an address that 
is only divisible by 4 instead of 8.

I can easily reproduce the problem.

A possible solution is to pad the three structures SHMCBHeader, 
SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For 
Subcache and Index this is already true by coincidence, SHMCBHeader 
needs another 4 Bytes.

I wonder what the right solution is. In the patch

http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch

I hard coded the padding, but I don't really like it, because it breaks 
if members are added to the struct. I could add a sizeof() test during 
startup or probably even compilation to warn or err, if the padding is 
wrong.

I see several recipes for alignment using pragmas and attribute, but all 
of them are compiler specific.

One could also wrap the struct in a wrapped struct, so that one could 
use the sizeof() of the inner struct to determine the padding of the 
outer struct. That would make the code convoluted.

I checked other parts of the code, but couldn't find a simple solution. 
Any hints how to do this nicely?

Regards,

Rainer

Re: How to align shm in an neat way?

[Joe Orton <jo...@redhat.com>]

On Mon, Aug 13, 2012 at 10:19:47PM +0200, Rainer Jung wrote:
> I went the "choose right alignment" way now:
> 
> http://people.apache.org/~rjung/patches/mod_socache_shmcb-alignment.patch
> 
> It actually wasn't that complicated.

Alignment problems never die with that code!

+1, that looks good, might be simpler using some #defines for the 
APR_ALIGN_DEFAULT(blah) values rather than inlining?

Regards, Joe

Re: How to align shm in an neat way?

[Rainer Jung <ra...@kippdata.de>]

On 13.08.2012 21:02, Rainer Jung wrote:
> On 13.08.2012 19:40, Jeff Trawick wrote:
>> On Mon, Aug 13, 2012 at 1:27 PM, Rainer Jung <ra...@kippdata.de>
>> wrote:
>>> On 13.08.2012 18:32, Jeff Trawick wrote:
>>>>
>>>> On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung <ra...@kippdata.de>
>>>> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> PR 53040 reveals, that mod_socache_shmcb has an alignment problem.
>>>>> One of
>>>>> the three structs mapped into shm contains an apr_time_t member,
>>>>> which at
>>>>> least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4
>>>>> Bytes.
>>>>>
>>>>> Currently everything is aligned for 4 Bytes, so we get bus
>>>>> errors/crashes
>>>>> when trying to assign the apr_time_t to an address that is only
>>>>> divisible
>>>>> by
>>>>> 4 instead of 8.
>>>>>
>>>>> I can easily reproduce the problem.
>>>>>
>>>>> A possible solution is to pad the three structures SHMCBHeader,
>>>>> SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For
>>>>> Subcache
>>>>> and Index this is already true by coincidence, SHMCBHeader needs
>>>>> another
>>>>> 4
>>>>> Bytes.
>>>>>
>>>>> I wonder what the right solution is. In the patch
>>>>>
>>>>> http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch
>>>>>
>>>>>
>>>>> I hard coded the padding, but I don't really like it, because it
>>>>> breaks
>>>>> if
>>>>> members are added to the struct. I could add a sizeof() test during
>>>>> startup
>>>>> or probably even compilation to warn or err, if the padding is wrong.
>>>>>
>>>>> I see several recipes for alignment using pragmas and attribute,
>>>>> but all
>>>>> of
>>>>> them are compiler specific.
>>>>>
>>>>> One could also wrap the struct in a wrapped struct, so that one
>>>>> could use
>>>>> the sizeof() of the inner struct to determine the padding of the outer
>>>>> struct. That would make the code convoluted.
>>>>>
>>>>> I checked other parts of the code, but couldn't find a simple
>>>>> solution.
>>>>> Any
>>>>> hints how to do this nicely?
>>>>
>>>>
>>>> APR_ALIGN_DEFAULT?
>>>
>>>
>>> I think it doesn't solve this problem, does it? It only gives me a
>>> need way
>>> to round up sizes to multiples of 8 bytes.
>>
>> It should be possible to use use APR_ALIGN_DEFAULT(sizeof(foo)) in
>> place of sizeof(foo), instead of adding explicit padding to the
>> structures. Any other pointer arithmetic which doesn't use sizeof(foo)
>> would also need to use the macro.
>
> Understood. Unfortunately currently the code doesn't really care about
> alignment. So it just puts structs SHMCBHeader, SHMCBSubcache and an
> array of SHMCBIndex directly after each other in shm. No sizeof() involved.
>
> So yes, option 3 would be to rewrite the code to calculate the
> gaps/padding between the structs using APR_ALIGN_DEFAULT() and adjust
> memory alignment in shm. I wanted to get around changing that part of
> the code ;)

I went the "choose right alignment" way now:

http://people.apache.org/~rjung/patches/mod_socache_shmcb-alignment.patch

It actually wasn't that complicated.

Regards,

Rainer

Re: How to align shm in an neat way?

[Rainer Jung <ra...@kippdata.de>]

On 13.08.2012 19:40, Jeff Trawick wrote:
> On Mon, Aug 13, 2012 at 1:27 PM, Rainer Jung <ra...@kippdata.de> wrote:
>> On 13.08.2012 18:32, Jeff Trawick wrote:
>>>
>>> On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung <ra...@kippdata.de>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
>>>> the three structs mapped into shm contains an apr_time_t member, which at
>>>> least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4
>>>> Bytes.
>>>>
>>>> Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
>>>> when trying to assign the apr_time_t to an address that is only divisible
>>>> by
>>>> 4 instead of 8.
>>>>
>>>> I can easily reproduce the problem.
>>>>
>>>> A possible solution is to pad the three structures SHMCBHeader,
>>>> SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For
>>>> Subcache
>>>> and Index this is already true by coincidence, SHMCBHeader needs another
>>>> 4
>>>> Bytes.
>>>>
>>>> I wonder what the right solution is. In the patch
>>>>
>>>> http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch
>>>>
>>>> I hard coded the padding, but I don't really like it, because it breaks
>>>> if
>>>> members are added to the struct. I could add a sizeof() test during
>>>> startup
>>>> or probably even compilation to warn or err, if the padding is wrong.
>>>>
>>>> I see several recipes for alignment using pragmas and attribute, but all
>>>> of
>>>> them are compiler specific.
>>>>
>>>> One could also wrap the struct in a wrapped struct, so that one could use
>>>> the sizeof() of the inner struct to determine the padding of the outer
>>>> struct. That would make the code convoluted.
>>>>
>>>> I checked other parts of the code, but couldn't find a simple solution.
>>>> Any
>>>> hints how to do this nicely?
>>>
>>>
>>> APR_ALIGN_DEFAULT?
>>
>>
>> I think it doesn't solve this problem, does it? It only gives me a need way
>> to round up sizes to multiples of 8 bytes.
>
> It should be possible to use use APR_ALIGN_DEFAULT(sizeof(foo)) in
> place of sizeof(foo), instead of adding explicit padding to the
> structures. Any other pointer arithmetic which doesn't use sizeof(foo)
> would also need to use the macro.

Understood. Unfortunately currently the code doesn't really care about 
alignment. So it just puts structs SHMCBHeader, SHMCBSubcache and an 
array of SHMCBIndex directly after each other in shm. No sizeof() involved.

So yes, option 3 would be to rewrite the code to calculate the 
gaps/padding between the structs using APR_ALIGN_DEFAULT() and adjust 
memory alignment in shm. I wanted to get around changing that part of 
the code ;)

Regards,

Rainer

Re: How to align shm in an neat way?

[Jeff Trawick <tr...@gmail.com>]

On Mon, Aug 13, 2012 at 1:27 PM, Rainer Jung <ra...@kippdata.de> wrote:
> On 13.08.2012 18:32, Jeff Trawick wrote:
>>
>> On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung <ra...@kippdata.de>
>> wrote:
>>>
>>> Hi,
>>>
>>> PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
>>> the three structs mapped into shm contains an apr_time_t member, which at
>>> least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4
>>> Bytes.
>>>
>>> Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
>>> when trying to assign the apr_time_t to an address that is only divisible
>>> by
>>> 4 instead of 8.
>>>
>>> I can easily reproduce the problem.
>>>
>>> A possible solution is to pad the three structures SHMCBHeader,
>>> SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For
>>> Subcache
>>> and Index this is already true by coincidence, SHMCBHeader needs another
>>> 4
>>> Bytes.
>>>
>>> I wonder what the right solution is. In the patch
>>>
>>> http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch
>>>
>>> I hard coded the padding, but I don't really like it, because it breaks
>>> if
>>> members are added to the struct. I could add a sizeof() test during
>>> startup
>>> or probably even compilation to warn or err, if the padding is wrong.
>>>
>>> I see several recipes for alignment using pragmas and attribute, but all
>>> of
>>> them are compiler specific.
>>>
>>> One could also wrap the struct in a wrapped struct, so that one could use
>>> the sizeof() of the inner struct to determine the padding of the outer
>>> struct. That would make the code convoluted.
>>>
>>> I checked other parts of the code, but couldn't find a simple solution.
>>> Any
>>> hints how to do this nicely?
>>
>>
>> APR_ALIGN_DEFAULT?
>
>
> I think it doesn't solve this problem, does it? It only gives me a need way
> to round up sizes to multiples of 8 bytes.

It should be possible to use use APR_ALIGN_DEFAULT(sizeof(foo)) in
place of sizeof(foo), instead of adding explicit padding to the
structures. Any other pointer arithmetic which doesn't use sizeof(foo)
would also need to use the macro.


>
> Regards,
>
> Rainer
>



-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: How to align shm in an neat way?

[Rainer Jung <ra...@kippdata.de>]

On 13.08.2012 18:32, Jeff Trawick wrote:
> On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung <ra...@kippdata.de> wrote:
>> Hi,
>>
>> PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
>> the three structs mapped into shm contains an apr_time_t member, which at
>> least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4 Bytes.
>>
>> Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
>> when trying to assign the apr_time_t to an address that is only divisible by
>> 4 instead of 8.
>>
>> I can easily reproduce the problem.
>>
>> A possible solution is to pad the three structures SHMCBHeader,
>> SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For Subcache
>> and Index this is already true by coincidence, SHMCBHeader needs another 4
>> Bytes.
>>
>> I wonder what the right solution is. In the patch
>>
>> http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch
>>
>> I hard coded the padding, but I don't really like it, because it breaks if
>> members are added to the struct. I could add a sizeof() test during startup
>> or probably even compilation to warn or err, if the padding is wrong.
>>
>> I see several recipes for alignment using pragmas and attribute, but all of
>> them are compiler specific.
>>
>> One could also wrap the struct in a wrapped struct, so that one could use
>> the sizeof() of the inner struct to determine the padding of the outer
>> struct. That would make the code convoluted.
>>
>> I checked other parts of the code, but couldn't find a simple solution. Any
>> hints how to do this nicely?
>
> APR_ALIGN_DEFAULT?

I think it doesn't solve this problem, does it? It only gives me a need 
way to round up sizes to multiples of 8 bytes.

Regards,

Rainer

Re: How to align shm in an neat way?

[Rainer Jung <ra...@kippdata.de>]

On 14.08.2012 23:20, Jim Jagielski wrote:
> slotmem handles this well, afaict ;)

The fix applied in r1373270 which Joe also had a look at is very similar 
to the handling in slotmem. I added some macros as proposed by Joe. 
Fortunately the code in shmcb already did all the address calculations 
using macros so I only had to add the alignment there.

Regards,

Rainer

Re: How to align shm in an neat way?

[Jim Jagielski <ji...@jaguNET.com>]

slotmem handles this well, afaict ;)

On Aug 13, 2012, at 12:32 PM, Jeff Trawick <tr...@gmail.com> wrote:

> On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung <ra...@kippdata.de> wrote:
>> Hi,
>> 
>> PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
>> the three structs mapped into shm contains an apr_time_t member, which at
>> least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4 Bytes.
>> 
>> Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
>> when trying to assign the apr_time_t to an address that is only divisible by
>> 4 instead of 8.
>> 
>> I can easily reproduce the problem.
>> 
>> A possible solution is to pad the three structures SHMCBHeader,
>> SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For Subcache
>> and Index this is already true by coincidence, SHMCBHeader needs another 4
>> Bytes.
>> 
>> I wonder what the right solution is. In the patch
>> 
>> http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch
>> 
>> I hard coded the padding, but I don't really like it, because it breaks if
>> members are added to the struct. I could add a sizeof() test during startup
>> or probably even compilation to warn or err, if the padding is wrong.
>> 
>> I see several recipes for alignment using pragmas and attribute, but all of
>> them are compiler specific.
>> 
>> One could also wrap the struct in a wrapped struct, so that one could use
>> the sizeof() of the inner struct to determine the padding of the outer
>> struct. That would make the code convoluted.
>> 
>> I checked other parts of the code, but couldn't find a simple solution. Any
>> hints how to do this nicely?
> 
> APR_ALIGN_DEFAULT?
> 
>> 
>> Regards,
>> 
>> Rainer
> 
> 
> 
> -- 
> Born in Roswell... married an alien...
> http://emptyhammock.com/
> 

Re: How to align shm in an neat way?

[Jeff Trawick <tr...@gmail.com>]

On Mon, Aug 13, 2012 at 12:30 PM, Rainer Jung <ra...@kippdata.de> wrote:
> Hi,
>
> PR 53040 reveals, that mod_socache_shmcb has an alignment problem. One of
> the three structs mapped into shm contains an apr_time_t member, which at
> least on Sparc is 8 Bytes, whereas for 32 bit builds long is only 4 Bytes.
>
> Currently everything is aligned for 4 Bytes, so we get bus errors/crashes
> when trying to assign the apr_time_t to an address that is only divisible by
> 4 instead of 8.
>
> I can easily reproduce the problem.
>
> A possible solution is to pad the three structures SHMCBHeader,
> SHMCBSubcache and SHMCBIndex to a multiple of 8 Bytes length. For Subcache
> and Index this is already true by coincidence, SHMCBHeader needs another 4
> Bytes.
>
> I wonder what the right solution is. In the patch
>
> http://people.apache.org/~rjung/patches/mod_socache_shmcb-padding.patch
>
> I hard coded the padding, but I don't really like it, because it breaks if
> members are added to the struct. I could add a sizeof() test during startup
> or probably even compilation to warn or err, if the padding is wrong.
>
> I see several recipes for alignment using pragmas and attribute, but all of
> them are compiler specific.
>
> One could also wrap the struct in a wrapped struct, so that one could use
> the sizeof() of the inner struct to determine the padding of the outer
> struct. That would make the code convoluted.
>
> I checked other parts of the code, but couldn't find a simple solution. Any
> hints how to do this nicely?

APR_ALIGN_DEFAULT?

>
> Regards,
>
> Rainer



-- 
Born in Roswell... married an alien...
http://emptyhammock.com/