[FFmpeg-devel] [PATCH] fft_tab_neon is not PIC enough

Reimar Döffinger Reimar.Doeffinger at gmx.de
Sun Jul 15 19:34:41 CEST 2012


On Sun, Jul 15, 2012 at 07:55:34AM -0700, John Reiser wrote:
> On 07/15/2012 02:04 AM, Reimar Döffinger wrote:
> > On 14 Jul 2012, at 17:24, John Reiser <jreiser at bitwagon.com> wrote:
> >> fft_tab_neon has addresses in .rodata, which is bad for shared libraries.
> > 
> > Could you please be more specific about what exactly "bad" means?
> 
> "Bad" means "does not fulfill the expectations of residing in a read-only
> segment."  The question becomes, "After what time does 'read-only' apply?"
> That is, what is the earliest time after which there are no more writes?

First, do you mean .rodata or .text? You switch between those.
First of all .rodata IMHO means "the application will not try to write into
that".
If the operating system/linker wants to write into it, that's something
they should deal with amongst themselves, it shouldn't really be the
task of the application to anticipate that (ideally).

> For a physical ROM, the time is immediately after hardware manufacturing.

As far as this case is concerned I would wonder if you'd really have
to/want to do runtime relocation and dynamic linking for code in ROM,
that seems like a extremely special case (though I guess you are aiming
at in-place execution of a full Linux system and e.g. EEPROM).

> My environment wants "no more writes to .text" after placement of the
> library into the file system of an operating system, and current code
> does not work with this restriction.  Current code requires writing
> those addresses during dlopen() or LoadLibrary() into a process.

Not that I see any issue with the assembler/linker detecting that and just not
putting that into the normal .rodata (and actually I thought they
already do, by creating a .rodata.relro subsection - though possibly
that's only handled at the compiler level yet).

> >> The attached patch provides the additional relocation by changing
> >> the table to an array of branch instructions.  The patch also
> >> checks the bounds of the index.  In Thumb mode the 11-bit displacement
> >> field of a short unconditional branch can span +/- 1024 words
> >> which is +/- 2048 bytes.  By moving the routine to near the middle,
> >> then the other code could double in size before exceeding
> >> the limit on span.
> > 
> > This seems extremely brittle to me, especially since I am not aware of a reason why the linker couldn't reorder the functions in whatever way it wants, so the absolute minimum would be supporting an offset that can handle the size of the whole .text segment (or reorganize the code so the linker will not be able to separate it, e.g. by putting everything in a single function, but that is still ugly and brittle).
> > For other architectures we have solved this problem by storing the difference between a label and the symbols in a table (those will always be real constants), at full 32 or 64 bits and manually add those onto the PC, then jump to the calculated address.
> 
> If you want the property that the linker can change placement of the fft<N>
> functions arbitrarily,

I'd rather say that with the trend to link-time optimization adding code
that can't handle the linker optimizing things seems like aiming the gun at
your foot.
And making it .data everywhere when it can be in .rodata on all systems
people use it commonly on seems overkill.
One thing that is trivial and I wouldn't have objections against is
changing const/endconst for these cases to e.g. relconst/endrelconst
so it's easy to change it all in one place.

> The current code is broken for my environment.  The patched code works in
> my environment and all other known current cases.

That makes me wonder what "all other know current cases" means, how much
did you test?
Because I have another minor nit, that the new code uses the 2-operand
"sub" mnemonic. I'm quite sure that the remaining code does not use that
because some assemblers still do not support it but only the 3-operand form.

> Enhancements are welcome, but DT_TEXTREL is prohibited.

I don't think I will have time, and also I certainly don't have
any system I could actually test the changes on.
So I was obviously hoping you'd be motivated to improve on it.
I don't claim to have the last call here, but IMHO as long
as it's only "your system" that is affected the requirements
for a patch are "pretty damn near perfect" (or at least unlikely to
cause any issues in any imaginable case) which I think yours
doesn't quite fulfill.


More information about the ffmpeg-devel mailing list