[FFmpeg-devel] [PATCH] Workaround Mac-OS x86_64 dynamic loader bug for Yasm generated code

[Loren Merritt]

On Sun, 22 Feb 2009, Diego Biurrun wrote:
> On Sun, Feb 22, 2009 at 01:04:35AM +0000, Loren Merritt wrote:
>> On Sat, 14 Feb 2009, Art Clarke wrote:
>>
>>> --- libavcodec/x86/fft_mmx.asm	(revision 16989)
>>> +++ libavcodec/x86/fft_mmx.asm	(working copy)
>>> @@ -446,8 +446,16 @@
>>> %endrep
>>> %undef n
>>>
>>> +%ifidn __OUTPUT_FORMAT__,macho64
>>> +; Putting this section in .text on x86_64 darwin causes problems
>>> +section .rodata align=16
>>> +%endif
>>> align 8
>>> dispatch_tab%3%2: pointer list_of_fft
>>> +%ifidn __OUTPUT_FORMAT__,macho64
>>> +; Set section back to .text
>>> +section .text align=16
>>> +%endif
>>
>> I'm confused. Wasn't this patch already applied in r17197, 28 hours before
>> Art's ping?
>
> No, that was a different thing, see what I attached.

So what does this patch fix compared to what was committed?

I see the following differences:

* align on section .rodata.
Fine, misalignment might very slightly impact speed. Though it won't 
actually take effect with macho and yasm<=0.7.2; that bug fix isn't in an 
official release yet.

* align on section .text.
That won't do anything: .text is already aligned due to its first 
declaration. As yasm says,
x86/fft_mmx.asm:467: warning: section flags ignored on section redeclaration

* section .text is conditional on macho64.
That won't do anything: there's no harm in an extra section statement that 
doesn't switch sections. Certainly doesn't justify spending 2 LOC on ifdefs.

* comments.
If you're going to comment it at all, at least say what the problems were, 
namely not supporting textrels.
And "Set section back to .text" adds absolutely nothing. It's just like 
the stereotypical "i++; /* increment i */".

--Loren Merritt