diff mbox

[FFmpeg-devel] lavc/sinewin: Do not declare tables as const

Message ID CAB0OVGq1WWmD+yTLYyXRss2B2HfOxXLMLnMLJZBTmZfO0-9m6A@mail.gmail.com
State Accepted
Headers show

Commit Message

Carl Eugen Hoyos Oct. 14, 2018, 9:51 p.m. UTC
2018-10-14 22:30 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> On Sun, Oct 14, 2018 at 10:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>>
>> Attached patch is supposed to fix ticket #7491, I currently
>> don't have gcc 8 to test myself.
>
> Only the 120 and 960 tables are affected by this bug because
> they are not being created by the hardcoded tables logic, yet
> this patch changes the const attribute for all tables, defeating
> one purpose of the hardcoded tables.

> Can we adjust this to only affect the two tables that need it

Did that in attached patch, please comment.

> or remove hardcoded tables for sinewin entirely, instead of leaving
> them half-baked, or add 120 and 960 to hardcoded tables generation?

Carl Eugen

Comments

Carl Eugen Hoyos Oct. 16, 2018, 9:58 p.m. UTC | #1
2018-10-14 23:51 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> 2018-10-14 22:30 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>> On Sun, Oct 14, 2018 at 10:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> wrote:
>>>
>>> Attached patch is supposed to fix ticket #7491, I currently
>>> don't have gcc 8 to test myself.
>>
>> Only the 120 and 960 tables are affected by this bug because
>> they are not being created by the hardcoded tables logic, yet
>> this patch changes the const attribute for all tables, defeating
>> one purpose of the hardcoded tables.
>
>> Can we adjust this to only affect the two tables that need it
>
> Did that in attached patch, please comment.

I will push this patch if there are no objections.

Carl Eugen
Carl Eugen Hoyos Oct. 19, 2018, 6:34 p.m. UTC | #2
2018-10-16 23:58 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>:
> 2018-10-14 23:51 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>> 2018-10-14 22:30 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>>> On Sun, Oct 14, 2018 at 10:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>> wrote:
>>>>
>>>> Attached patch is supposed to fix ticket #7491, I currently
>>>> don't have gcc 8 to test myself.
>>>
>>> Only the 120 and 960 tables are affected by this bug because
>>> they are not being created by the hardcoded tables logic, yet
>>> this patch changes the const attribute for all tables, defeating
>>> one purpose of the hardcoded tables.
>>
>>> Can we adjust this to only affect the two tables that need it
>>
>> Did that in attached patch, please comment.
>
> I will push this patch if there are no objections.

Patch applied.

Carl Eugen
Paul B Mahol Oct. 19, 2018, 6:35 p.m. UTC | #3
On 10/19/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-10-16 23:58 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>> 2018-10-14 23:51 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>>> 2018-10-14 22:30 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>>>> On Sun, Oct 14, 2018 at 10:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>> wrote:
>>>>>
>>>>> Attached patch is supposed to fix ticket #7491, I currently
>>>>> don't have gcc 8 to test myself.
>>>>
>>>> Only the 120 and 960 tables are affected by this bug because
>>>> they are not being created by the hardcoded tables logic, yet
>>>> this patch changes the const attribute for all tables, defeating
>>>> one purpose of the hardcoded tables.
>>>
>>>> Can we adjust this to only affect the two tables that need it
>>>
>>> Did that in attached patch, please comment.
>>
>> I will push this patch if there are no objections.
>
> Patch applied.

Very ugly solution.
Carl Eugen Hoyos Oct. 19, 2018, 6:40 p.m. UTC | #4
2018-10-19 20:35 GMT+02:00, Paul B Mahol <onemda@gmail.com>:
> On 10/19/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2018-10-16 23:58 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>>> 2018-10-14 23:51 GMT+02:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>:
>>>> 2018-10-14 22:30 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>>>>> On Sun, Oct 14, 2018 at 10:18 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>> wrote:
>>>>>>
>>>>>> Attached patch is supposed to fix ticket #7491, I currently
>>>>>> don't have gcc 8 to test myself.
>>>>>
>>>>> Only the 120 and 960 tables are affected by this bug because
>>>>> they are not being created by the hardcoded tables logic, yet
>>>>> this patch changes the const attribute for all tables, defeating
>>>>> one purpose of the hardcoded tables.
>>>>
>>>>> Can we adjust this to only affect the two tables that need it
>>>>
>>>> Did that in attached patch, please comment.
>>>
>>> I will push this patch if there are no objections.
>>
>> Patch applied.
>
> Very ugly solution.

I don't disagree but keeping the hard-to-debug crash is not a
solution imo.

Carl Eugen
diff mbox

Patch

From 55fc231778360c921827915e6aa8a2b1a592dfdd Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Sun, 14 Oct 2018 23:48:23 +0200
Subject: [PATCH] lavc/sinewin: Do not declare AAC 120/960 tables as const.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

ff_sine_window_init() is writing to these tables causing
a crash if compiled with gcc 8 and lto.

Analyzed by Martin Liška in:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85132

Fixes ticket #7491.
---
 libavcodec/sinewin.h                   |    7 +++++--
 libavcodec/sinewin_tablegen.h          |    4 ++--
 libavcodec/sinewin_tablegen_template.c |    2 ++
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/libavcodec/sinewin.h b/libavcodec/sinewin.h
index 6b97a71..329e9bb 100644
--- a/libavcodec/sinewin.h
+++ b/libavcodec/sinewin.h
@@ -38,6 +38,9 @@ 
 #define SINETABLE(size) \
     SINETABLE_CONST DECLARE_ALIGNED(32, INTFLOAT, AAC_RENAME(ff_sine_##size))[size]
 
+#define SINETABLE120960(size) \
+    DECLARE_ALIGNED(32, INTFLOAT, AAC_RENAME(ff_sine_##size))[size]
+
 /**
  * Generate a sine window.
  * @param   window  pointer to half window
@@ -52,11 +55,11 @@  void AAC_RENAME(ff_init_ff_sine_windows)(int index);
 
 extern SINETABLE(  32);
 extern SINETABLE(  64);
-extern SINETABLE( 120);
+extern SINETABLE120960(120);
 extern SINETABLE( 128);
 extern SINETABLE( 256);
 extern SINETABLE( 512);
-extern SINETABLE( 960);
+extern SINETABLE120960(960);
 extern SINETABLE(1024);
 extern SINETABLE(2048);
 extern SINETABLE(4096);
diff --git a/libavcodec/sinewin_tablegen.h b/libavcodec/sinewin_tablegen.h
index 0fa3561..dc52234 100644
--- a/libavcodec/sinewin_tablegen.h
+++ b/libavcodec/sinewin_tablegen.h
@@ -32,8 +32,8 @@ 
 #include "libavutil/common.h"
 
 #if !USE_FIXED
-SINETABLE( 120);
-SINETABLE( 960);
+SINETABLE120960(120);
+SINETABLE120960(960);
 #endif
 #if !CONFIG_HARDCODED_TABLES
 SINETABLE(  32);
diff --git a/libavcodec/sinewin_tablegen_template.c b/libavcodec/sinewin_tablegen_template.c
index 43ce1ba..b8eb407 100644
--- a/libavcodec/sinewin_tablegen_template.c
+++ b/libavcodec/sinewin_tablegen_template.c
@@ -33,6 +33,8 @@ 
 #define SINETABLE_CONST
 #define SINETABLE(size) \
     INTFLOAT AAC_RENAME(ff_sine_##size)[size]
+#define SINETABLE120960(size) \
+    INTFLOAT AAC_RENAME(ff_sine_##size)[size]
 #define FF_ARRAY_ELEMS(a) (sizeof(a) / sizeof((a)[0]))
 #include "sinewin_tablegen.h"
 #include "tableprint.h"
-- 
1.7.10.4