[FFmpeg-devel] PIC and YASM

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:

> On Tue, Nov 10, 2009 at 12:21:28AM +0000, M?ns Rullg?rd wrote:
>> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
>> 
>> > Ok, after all the discussion I have only one change left to suggest
>> > (well, actually I had it suggested a few times, but I am not sure if
>> > someone is against it or not):
>> > Index: configure
>> > ===================================================================
>> > --- configure   (revision 20491)
>> > +++ configure   (working copy)
>> > @@ -1876,6 +1876,11 @@
>> >
>> >  enable $arch $subarch
>> >  enabled spic && enable pic
>> > +# This is the same check as used in libavutil/internal.h
>> 
>> Shouldn't that one go away?
>
> Sooner or later I think yes. However I don't like too much doing it in
> the same commit.

If it has the same effect, it should be done in one place only.  As
long as it stays, we won't notice if the new check is insufficient
somewhere (not that I can imagine how that could be).

>> > +# to enable RIP-relative addressing for x86_64 inline asm.
>> > +# This also makes compilation work out-of-the-box for systems
>> > +# like hardened Gentoo where the compiler generates PIC/PIE by default.
>> 
>> This comment seems like a roundabout way of saying
>> # Check if the compiler uses PIC by default
>
> Well, that no longer explains _why_.

We can't have an essay explaining every line of code.

> Also I'd think you'd have to be a bit dense to not realize it checks for

So drop it entirely.

> PIC (btw. it also catches --extra-cflags=-fPIC, though people really
> should be using --enable-pic).
> I really like to have comments that mention the affected systems because
> I have lately spent too much time search which kind of system on earth
> some hack is supposed to be for.

Did you try git blame?  We have commit messages for a reason.

> So here's one more try, though since you'll be maintaining it in the end
> I don't mind going with the one-line comment you gave above instead...
>
> Index: configure
> ===================================================================
> --- configure   (revision 20491)
> +++ configure   (working copy)
> @@ -1876,6 +1876,12 @@
>
>  enable $arch $subarch
>  enabled spic && enable pic
> +# Check if the compiler uses PIC so that the YASM code is compiled
> +# appropriately, fixing static compilation on x86_64 with PIE-enabled
> +# compilers (e.g. hardened Gentoo).
> +# This is the same check as used in libavutil/internal.h
> +# to enable RIP-relative addressing for x86_64 inline asm.

This comment talks about *one* effect of having this line here.  It is
in no way specific to yasm code, even if that is the only thing
currently affected by it.  Every line in the script is there to make
something or other work, yet there isn't a comment next to each
explaining it in detail.

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