[FFmpeg-devel] [PATCH] Fix missing used attribute for inline assembly variables

Thomas Köppe tkoeppe at google.com
Thu Nov 9 21:03:59 EET 2017


Thank you, I'll package this up as a patch and send it as a separate mail.

On 9 November 2017 at 11:52, Teresa Johnson <tejohnson at google.com> wrote:

> I implemented this change to add a new macro. I tried to find the
> variables used in MANGLE and change those to use the new
> DECLARE_ASM_ALIGNED, and the build succeeds with these changes. New changes
> are in cl/172133815 <https://critique.corp.google.com/#review/172133815>.
>
> Teresa
>
> On Wed, Nov 1, 2017 at 7:25 AM, Teresa Johnson <tejohnson at google.com>
> wrote:
>
>>
>>
>> On Tue, Oct 31, 2017 at 5:42 PM, Michael Niedermayer <
>> michael at niedermayer.cc> wrote:
>>
>>> Hi
>>>
>>> On Tue, Oct 31, 2017 at 04:29:18PM +0000, Teresa Johnson wrote:
>>> > It's needed for the same reason the used attribute was already added
>>> to the
>>> > "static const" variables. For those, when building with just -O2, they
>>> > could be removed by optimization since they had local (file) scope,
>>> and we
>>> > couldn't see the uses in the inline assembly (without the used
>>> attribute).
>>> > With ThinLTO we have whole program scope, so even though they are
>>> > non-static and have global scope we can do dead code elimination on
>>> them if
>>> > we don't see that they are used.
>>>
>>> currently we add "used" to DECLARE_ASM_CONST()
>>> which is specific to inline asm use.
>>>
>>> DECLARE_ALIGNED() is not specific to use in asm.
>>>
>>> For DECLARE_ASM_CONST() the "used" is unneeded only in the subset of
>>> inline asm cases where it is accessed through the asm operands like:
>>>     __asm__ volatile(
>>>         "movq          %0, %%mm7    \n\t"
>>>         "movq          %1, %%mm6    \n\t"
>>>         ::"m"(red_16mask),"m"(green_16mask));
>>>
>>> The compiler has full vissibility here of the access and if it removes
>>> it its a  compiler bug.
>>>
>>> this is compared to code like:
>>>  "pand "MANGLE(mask24l)", %%mm0\n\t"
>>>
>>> Here the compiler has no easy vissibility of the use, it is part of
>>> the asm string.
>>>
>>> and comparing this to DECLARE_ALIGNED(), the big difference is
>>> that DECLARE_ALIGNED() is used by plain C code which never should need
>>> "used". So adding "used" to DECLARE_ALIGNED() would add alot more
>>> "unused detection overriding" than what is needed
>>>
>>
>> Perhaps then an additional macro is needed for variables that are
>> currently DECLARED_ALIGNED but used by MANGLE, which adds the used
>> attribute. What do you suggest?
>> Teresa
>>
>>
>>>
>>>
>>>
>>> >
>>> > Teresa
>>> >
>>> > On Tue, Oct 31, 2017 at 4:19 PM, Michael Niedermayer
>>> <michael at niedermayer.cc
>>> > > wrote:
>>> >
>>> > > On Tue, Oct 31, 2017 at 03:52:14PM +0000, Thomas Köppe wrote:
>>> > > > +Teresa, who drafted the patch initially.
>>> > > >
>>> > > > On 31 October 2017 at 15:38, Michael Niedermayer
>>> <michael at niedermayer.cc
>>> > > >
>>> > > > wrote:
>>> > > >
>>> > > > > On Tue, Oct 31, 2017 at 12:16:18PM +0000, Thomas Köppe wrote:
>>> > > > > > Variables used in inline assembly need to be marked with
>>> > > > > attribute((used)).
>>> > > > >
>>> > > > > This should not be the case.
>>> > > > > Variables referenced through in/out operands should not need
>>> that.
>>> > > > > Only ones accessed directly bypassing operands should need this
>>> > > > >
>>> > > >
>>> > > > I've added Teresa to the thread, who initially analyzed the
>>> problem. (For
>>> > > > background on ThinLTO, see here cppcon talk:
>>> > > > https://www.youtube.com/watch?v=p9nH2vZ2mNo.)
>>> > > >
>>> > > > [...]
>>> > > > > > -    #define DECLARE_ALIGNED(n,t,v)      t __attribute__
>>> ((aligned
>>> > > (n)))
>>> > > > > v
>>> > > > > > +    #define DECLARE_ALIGNED(n,t,v)      t av_used
>>> __attribute__
>>> > > > > ((aligned (n))) v
>>> > > > >
>>> > > > > which variables actually are affected/ need this ?
>>> > > > >
>>> > > >
>>> > > > Without this annotation, and when compiling and linking with
>>> ThinLTO, I
>>> > > get
>>> > > > an undefined reference to "ff_h264_cabac_tables", and also to
>>> > > > "ff_pw_96", "ff_pw_53",
>>> > > > "ff_pw_42", "ff_w1111" and many more.
>>> > >
>>> > > i guess these are all accessed directly, like through MANGLE()
>>> > >
>>> > >
>>> > > >
>>> > > >
>>> > > > > Marking all aligned variables as used makes it harder to spot
>>> unused
>>> > > > > variables leading to accumulation of cruft
>>> > > > >
>>> > > >
>>> > > > I see. Maybe we can annotate only the affected variables more
>>> granularly?
>>> > > > _______________________________________________
>>> > > > ffmpeg-devel mailing list
>>> > > > ffmpeg-devel at ffmpeg.org
>>> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> > >
>>> > > --
>>> > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7
>>> 87040B0FAB
>>> > >
>>> > > Avoid a single point of failure, be that a person or equipment.
>>> > >
>>> >
>>> >
>>> >
>>> > --
>>> > Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>>> 408-460-2413
>>> > _______________________________________________
>>> > ffmpeg-devel mailing list
>>> > ffmpeg-devel at ffmpeg.org
>>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> --
>>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>>
>>> I am the wisest man alive, for I know one thing, and that is that I know
>>> nothing. -- Socrates
>>>
>>
>>
>>
>> --
>> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>> 408-460-2413 <(408)%20460-2413>
>>
>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
>  408-460-2413
>


More information about the ffmpeg-devel mailing list