[Ffmpeg-devel] Re: [PATCH] Machine endian bytestream functions

[Måns Rullgård]

Ramiro Ribeiro Polla <ramiro at lisha.ufsc.br> writes:

> Hi,
>
> M?ns Rullg?rd wrote:
>> Ramiro Ribeiro Polla <ramiro at lisha.ufsc.br> writes:
>>
>>
>>> functional.diff changed configure to use the new check_exec_crash
>>> function. This should detect if unaligned data access doesn't crash,
>>> and if it returns the correct non-rotated values (as I read in [1]).
>>> depends on cosmetics.diff from previous message.
>>>
>>> Ramiro Polla
>>> [1] http://www.arm.com/support/faqdev/1469.html
>>>
>>> Index: configure
>>> ===================================================================
>>> --- configure	(revision 8727)
>>> +++ configure	(working copy)
>>> @@ -594,6 +594,7 @@
>>>      pp
>>>      protocols
>>>      swscaler
>>> +    unaligned_access
>>>      vhook
>>>      v4l
>>>      v4l2
>>>
>>
>> This should be in the HAVE_* list.  It's not user-configurable.
>>
> Ok.
> Should ebp/ebx_available also be set to HAVE then?

Yes.  This should be done separately though.

>>> @@ -1439,6 +1440,15 @@
>>>  EOF
>>>  fi
>>>  +check_exec_crash <<EOF && enable unaligned_access
>>> +// test for unaligned data access
>>>
>>
>> That comment isn't necessary.
>>
> Yes, it is. Not exactly the comment, but the #include can't be there
> in the first line, since it expands to "{ #include..." in
> check_exec_crash, and that doesn't compile.

Didn't think of that when I wrote it.  It's easy enough to fix.

>>> +#include <inttypes.h>
>>> +    uint8_t data[5];
>>> +    uint32_t *pdata = (uint32_t*)(((intptr_t)data&1)?data:(data+1));
>>
>> uint32_t data[2];
>> uint32_t *pdata = (uint32_t*)(uint8_t*)data+1;
>>
> The data _must_ be unaligned. If the stack isn't aligned, and it just
> happens that data+1 becomes aligned, the test doesn't serve its
> purpose.

The compiler is required to align uint32_t to 4 bytes.  Adding one
will necessarily produce an unaligned address.  All 32-bit ABIs
require a stack alignment of at least 4 bytes.

>>> +    *pdata = 0x01234567;
>>> +    return !(*pdata == 0x01234567);
>>> +EOF
>>>
>>
>> This entire check is actually rather pointless.  On many systems that
>> do not support unaligned accesses, the OS will by default fix it up in
>> a trap handler.  There is no way to detect this, but it will run very
>> slowly.  Furthermore, even on x86 aligned accesses are faster than
>> unaligned.
> Is there no way to trap this exception ourselves?

Not portably and reliably.

> Do you suggest this test be dropped and we make a list of
> architectures where unaligned access is possible and somewhat faster
> than doing the shifting and adding? (this is what the previously
> ok'd patch did)

No, I'm suggesting this:

>> I'm thinking the unaligned16/32/64 macros from bitstream.h should be
>> renamed and moved here instead.
>>
> I'll take a look.

This lets the compiler choose whatever access method is appropriate,
and saves us the trouble of maintaining a fragile list.

-- 
M?ns Rullg?rd
mans at mansr.com