[FFmpeg-devel,v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.

Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
---
 libavcodec/mips/simple_idct_mmi.c | 2 +-
 libavutil/mips/mmiutils.h         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

On 26.07.2019, at 07:18, Shiyou Yin <yinshiyou-hf@loongson.cn> wrote:

> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.

Looks good to me if standard DECLARE_ALIGNED should work for stack on MIPS.
(on x86 it used to be possible for the stack pointer to only be 8-byte aligned, in which case DECLARE_ALIGNED could not actually provide 16-byte aligned stack variables, so there were special macros - I have not checked how it works nowadays or if MIPS has that issue).
Well, I guess regardless of that, this is better than before, so should be fine to apply either way.

> ---
> libavcodec/mips/simple_idct_mmi.c | 2 +-
> libavutil/mips/mmiutils.h         | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/mips/simple_idct_mmi.c b/libavcodec/mips/simple_idct_mmi.c
> index 7f4bb74..73d797f 100644
> --- a/libavcodec/mips/simple_idct_mmi.c
> +++ b/libavcodec/mips/simple_idct_mmi.c
> @@ -39,7 +39,7 @@
> #define COL_SHIFT 20
> #define DC_SHIFT 3
> 
> -DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
> +DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
>     W4,  W2,  W4,  W6,
>     W1,  W3,  W5,  W7,
>     W4,  W6, -W4, -W2,
> diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
> index 05f6b31..14b6d20 100644
> --- a/libavutil/mips/mmiutils.h
> +++ b/libavutil/mips/mmiutils.h
> @@ -205,7 +205,7 @@
>  * backup register
>  */
> #define BACKUP_REG \
> -  double temp_backup_reg[8];                                    \
> +  DECLARE_ALIGNED(16, double, temp_backup_reg)[8];              \
>   if (_MIPS_SIM == _ABI64)                                      \
>     __asm__ volatile (                                          \
>       "gssqc1       $f25,      $f24,       0x00(%[temp])  \n\t" \
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

>-----Original Message-----
>From: ffmpeg-devel-bounces@ffmpeg.org [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of
>Reimar D?ffinger
>Sent: Sunday, July 28, 2019 6:37 AM
>To: FFmpeg development discussions and patches
>Subject: Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
>
>On 26.07.2019, at 07:18, Shiyou Yin <yinshiyou-hf@loongson.cn> wrote:
>
>> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
>
>Looks good to me if standard DECLARE_ALIGNED should work for stack on MIPS.
>(on x86 it used to be possible for the stack pointer to only be 8-byte aligned, in which case
>DECLARE_ALIGNED could not actually provide 16-byte aligned stack variables, so there were special
>macros - I have not checked how it works nowadays or if MIPS has that issue).
>Well, I guess regardless of that, this is better than before, so should be fine to apply either way.
>
DECLARE_ALIGNED is defined in ' libavutil/mem.h ' and related to compiler. No matter mips or x86,
it's definition is ' #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v' when build 
with gcc or clang (Specific implementation within the compiler is not considered here.).
In libavcodec/x86, DECLARE_ALIGNED is used to define 8/16/32-byte aligned variable too.

Here is a test on mips.
1) Defined variable with 'double temp[8];', and compiled with 'gcc -fstack-protector-all'.
In this case, address of temp is not 16-byte aligned and gssqc1/gslqc1 caused instruction exception. 
2) Replacing ' double temp[8];' with ' double __attribute__((aligned(16))) __back_temp[8];'.
Address of temp is 16-byte aligned and gssqc1/gslqc1 works properly.

There is a typo in the commit message and have revised in v4. Please help to apply.

On 29.07.2019, at 11:54, "Shiyou Yin" <yinshiyou-hf@loongson.cn> wrote:
>> 
> DECLARE_ALIGNED is defined in ' libavutil/mem.h ' and related to compiler. No matter mips or x86,
> it's definition is ' #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v' when build 
> with gcc or clang (Specific implementation within the compiler is not considered here.).
> In libavcodec/x86, DECLARE_ALIGNED is used to define 8/16/32-byte aligned variable too.

The aligned attribute does not work reliably with stack variables in some cases.
Compare with other code, I think you need to use LOCAL_ALIGNED_16 for the stack variable.
Yes, it might work in your test even with DECLARE_ALIGNED, but it might not be robust.

>-----Original Message-----
>From: ffmpeg-devel-bounces@ffmpeg.org [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of
>Reimar D?ffinger
>Sent: Tuesday, July 30, 2019 2:54 AM
>To: FFmpeg development discussions and patches
>Subject: Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
>
>On 29.07.2019, at 11:54, "Shiyou Yin" <yinshiyou-hf@loongson.cn> wrote:
>>>
>> DECLARE_ALIGNED is defined in ' libavutil/mem.h ' and related to compiler. No matter mips or x86,
>> it's definition is ' #define DECLARE_ALIGNED(n,t,v)      t __attribute__ ((aligned (n))) v' when build
>> with gcc or clang (Specific implementation within the compiler is not considered here.).
>> In libavcodec/x86, DECLARE_ALIGNED is used to define 8/16/32-byte aligned variable too.
>
>The aligned attribute does not work reliably with stack variables in some cases.
>Compare with other code, I think you need to use LOCAL_ALIGNED_16 for the stack variable.
>Yes, it might work in your test even with DECLARE_ALIGNED, but it might not be robust.

You are right, LOCAL_ALIGNED_16 might be more robust.
In v5 LOCAL_ALIGNED_16 is used for the stack variables .