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

[Teresa Johnson]

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.

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