Home | Docs | Forums | Lists | Bugs | Planet | Store | GMN | Get Gentoo!

Full Text Bug Listing

while merging...

# TEXTREL  ./usr/lib/libmp3lame.so.0.0.0
--------------------------------
Text relocations require a lot of extra work to be preformed by the
dynamic linker which will cause serious performance impact on IA-32
and might not function properly on other architectures hppa for example.
--------------------------------

These are ELF Q/A problems.

eu-findtextrel reports that the the file containing the function
choose_table_MMX or the function has_MMX_nasm are not fully pic aware.

I first tested the --with-pic without success. I then tested without the
$(use_enable x86 nasm) as the other tool hinted towards nasm already and
liblame.so is now free of text relocations.

Till such time as the asm used within libmp3lame.so is pic aware and
free of text relocations we should not enable/force the use of nasm by
default.

ferringb helped me test lame without the inline nasm on irc
and he said "works, no discernible difference mp3 sounds fine also"

fixed in cvs, thanks.

Reopening.

We could benefit from a fix instead of disabling it like it has been done.
I was thinking about adding a mmx useflag but this would cause the textrels to
reappear. If someone has the courrage to propose a fix as it then has textrels
and, as a bonus, exec stacks.

For quick benchs of the mmx vs non mmx version, see bug #230860 comment #4

Created an attachment (id=160249) [details]
textrel fix for lame 3.98

first attempt, someone please go through it and check that i got all the
modifications right ;). on a sidenote, lame doesn't have asm code for amd64, so
don't enable it there (re bug #230860).

as for GNU_STACK, nasm.h can produce a proper one but it's guarded by #ifdef
LINUX and i haven't seen that defined anywhere, i don't know where it was
supposed to come from, nasm doesn't define it per se for sure. so you can just
define it yourself or just remove the #ifdef altogether.

note that the textrel patch is also ELF specific, yet it's not guarded by
#ifdef's, so it's not ready for upstream as it is.

(In reply to comment #3)
> Created an attachment (id=160249) [edit] [details]
> textrel fix for lame 3.98
> 
> first attempt, someone please go through it and check that i got all the
> modifications right ;). 

Thanks, but it gives me a segfault ;)

(gdb) run audiodump.wav
Starting program: /usr/bin/lame audiodump.wav
LAME 3.98 32bits (http://www.mp3dev.org/)
CPU features: MMX (ASM used), 3DNow! (ASM used), SSE (ASM used), SSE2
Using polyphase lowpass filter, transition band: 16538 Hz - 17071 Hz
Encoding audiodump.wav to audiodump.wav.mp3
Encoding as 44.1 kHz j-stereo MPEG-1 Layer III (11x) 128 kbps qval=3
    Frame          |  CPU time/estim | REAL time/estim | play/CPU |    ETA 
     0/10322  ( 0%)|    0:00/    0:00|    0:00/    0:00|   0.0000x|    0:00 
04:29-----------------------------------------------------------------------------------------------------------------------
   kbps      %     %
    0.0           
Program received signal SIGSEGV, Segmentation fault.
0xf7f770f8 in choose_table_MMX.H_dual_lp1 () from /usr/lib/libmp3lame.so.0
(gdb) bt
#0  0xf7f770f8 in choose_table_MMX.H_dual_lp1 () from /usr/lib/libmp3lame.so.0
#1  0x00000086 in ?? ()
#2  0xf7f8bff4 in ?? () from /usr/lib/libmp3lame.so.0
#3  0x0000001e in ?? ()
#4  0xf7f7265c in noquant_count_bits (gfc=0x971f470, gi=0x9729a34,
prev_noise=0x0) at takehiro.c:734
#5  0xf7f72ade in count_bits (gfc=0x971f470, xr=0xff7c2d74, gi=0x9729a34,
prev_noise=0x0) at takehiro.c:788
#6  0xf7f6b620 in outer_loop (gfp=0x971e860, cod_info=0x9729a34,
l3_xmin=0xff7c3674, xrpow=0xff7c2d74, ch=0, targ_bits=855)
    at quantize.c:364
#7  0xf7f6ca80 in CBR_iteration_loop (gfp=0x971e860, pe=0xff7c864c,
ms_ener_ratio=0xff7c867c, ratio=0xff7c76ec)
    at quantize.c:2017
#8  0xf7f5adbd in lame_encode_mp3_frame (gfp=0x971e860, inbuf_l=0x971f480,
inbuf_r=0x97232c0, mp3buf=0xff7ec770 "��\220d", 
    mp3buf_size=147456) at encoder.c:535
#9  0xf7f5f0d3 in lame_encode_buffer_sample_t (gfp=0x971e860,
buffer_l=0x9737090, buffer_r=0x9738298, nsamples=1152, 
    mp3buf=0xff7ec770 "��\220d", mp3buf_size=147456) at lame.c:1422
#10 0x0804b2f7 in lame_encoder (gf=0x971e860, outf=0x971eb30, nogap=0,
inPath=0xff8dac99 "audiodump.wav", 
    outPath=0xff8dcc9b "audiodump.wav.mp3") at main.c:485
#11 0x0804c599 in main (argc=-7478668, argv=0x0) at main.c:855


(gdb) x/8i $pc
0xf7f770f8 <choose_table_MMX.H_dual_lp1>:       movq   (%edx,%ecx,1),%mm0
0xf7f770fc <choose_table_MMX.H_dual_lp1+4>:     movq   0x8(%edx,%ecx,1),%mm1
0xf7f77101 <choose_table_MMX.H_dual_lp1+9>:     packssdw %mm1,%mm0
0xf7f77104 <choose_table_MMX.H_dual_lp1+12>:    movq   %mm0,%mm2
0xf7f77107 <choose_table_MMX.H_dual_lp1+15>:    paddusw %mm5,%mm0
0xf7f7710a <choose_table_MMX.H_dual_lp1+18>:    pcmpgtw %mm6,%mm2
0xf7f7710d <choose_table_MMX.H_dual_lp1+21>:    pmaddwd %mm3,%mm0
0xf7f77110 <choose_table_MMX.H_dual_lp1+24>:    movd   %mm0,%ebx

(gdb) i r
eax            0x6      6
ecx            0x14c50  85072
edx            0x972a3ac        158507948
ebx            0xfffffef0       -272
esp            0xff7c0bb0       0xff7c0bb0
ebp            0xf7f8bff4       0xf7f8bff4
esi            0xdddfd5bf       -572533313
edi            0x971f470        158463088
eip            0xf7f770f8       0xf7f770f8 <choose_table_MMX.H_dual_lp1>
eflags         0x10206  [ PF IF RF ]
cs             0x23     35
ss             0x2b     43
ds             0x2b     43
es             0x2b     43
fs             0x0      0
gs             0x63     99


please let me know if you need additional information.

Created an attachment (id=160547) [details]
fixed lame pic fix patch

thanks, it seems i got a bit overzealous in adjusting the stack at one point
;), should work now.

one more thing: it seems that some dependencies are incorrect as i can't build
it with -j3.

(In reply to comment #6)
> Created an attachment (id=160547) [edit] [details]
> fixed lame pic fix patch
> 
> thanks, it seems i got a bit overzealous in adjusting the stack at one point
> ;), should work now.

Thanks I've committed it. It's still not enabled but people can test it with
EXTRA_ECONF=--enable-nasm

How do you think we should fix the last remaining issue, exec stacks ?  I've
replaced the %ifdef LINUX by %ifidn __OUTPUT_FORMAT__,elf in nasm.h and it
seems fine.

(In reply to comment #7)
> one more thing: it seems that some dependencies are incorrect as i can't build
> it with -j3.

Weird, it never failed here. Though I have only one cpu ;)
If you happen to understand the problem, please let me know.

(In reply to comment #8)
> Thanks I've committed it. It's still not enabled but people can test it with
> EXTRA_ECONF=--enable-nasm
> 
> How do you think we should fix the last remaining issue, exec stacks ?  I've
> replaced the %ifdef LINUX by %ifidn __OUTPUT_FORMAT__,elf in nasm.h and it
> seems fine.

that's fine with me, the 'proper' approach really depends on whether and how
you want to push this upstream (as they care about more systems than gentoo
does).

> (In reply to comment #7)
> > one more thing: it seems that some dependencies are incorrect as i can't build
> > it with -j3.
> 
> Weird, it never failed here. Though I have only one cpu ;)
> If you happen to understand the problem, please let me know.

try it on a real SMP box ;). anyway, i didn't bother to look, i think you can
disable parallel make in portage for now.

Created an attachment (id=160597) [details]
one more fix

should fix the crash reported in #230860.

Thanks for the updated patch. It doesn't segfault anymore.

patch updated, thanks

and parallel make should be fine also

it seems we're all good now, I'm going to commit this

(In reply to comment #3)
> on a sidenote, lame doesn't have asm code for amd64, so
> don't enable it there (re bug #230860).

@amd64: FYI I'm going to use.mask mmx useflag for lame on your profiles, it
won't be built anyway so it doesn't hurt but that'll save people from
installing nasm if they don't have it.

all done, thanks everyone!

Does the PaX team plan to get this upstream?

(In reply to comment #15)
> Does the PaX team plan to get this upstream?

no, i don't (not enough time to fight these kinds of things through ;), so feel
free.

Congrats, your changes made it into lame 3.98.2:
sk@jigsaw:~/tmp/lame-398-2-tet$ scanelf -qe frontend/lame
sk@jigsaw:~/tmp/lame-398-2-tet$ scanelf -qt frontend/lame

2008-08-31 19:35  robert

        * libmp3lame/i386/nasm.h (lame3_98):

        fixing the fix to get it assemble for ELF again

2008-08-31 12:26  robert

        * libmp3lame/i386/: choose_table.nas, fft3dn.nas, fftsse.nas,
          nasm.h (lame3_98):

        fixing none PIC assembling problem

2008-08-25 12:57  rbrito

        * debian/changelog, libmp3lame/i386/choose_table.nas,
          libmp3lame/i386/fft3dn.nas, libmp3lame/i386/fftsse.nas
          (lame3_98):

        Fix text relocations and update debian changelog.
        Thanks to the PaX Team and Gentoo people.

2008-08-25 12:43  rbrito

        * libmp3lame/i386/nasm.h (lame3_98):

        Fix to executable stack (thanks to Gentoo people)

2008-08-25 12:38  rbrito

        * libmp3lame/i386/Makefile.am (lame3_98):

        Fixes building of asm with libtool 2.2 (thanks to Gentoo people)

(In reply to comment #17)
> Congrats, your changes made it into lame 3.98.2:
> sk@jigsaw:~/tmp/lame-398-2-tet$ scanelf -qe frontend/lame
> sk@jigsaw:~/tmp/lame-398-2-tet$ scanelf -qt frontend/lame

thanks, any chance to push this version into portage then?

(In reply to comment #18)
> (In reply to comment #17)
> > Congrats, your changes made it into lame 3.98.2:
> > sk@jigsaw:~/tmp/lame-398-2-tet$ scanelf -qe frontend/lame
> > sk@jigsaw:~/tmp/lame-398-2-tet$ scanelf -qt frontend/lame
> 
> thanks, any chance to push this version into portage then?

*hrm...
It's been in portage for a few days now:
*lame-3.98.2 (23 Sep 2008)

  23 Sep 2008; Peter Alfredsen <loki_val@gentoo.org> +lame-3.98.2.ebuild:
  Bump to 3.98.2, bug 238443. Drop a bunch of patches that were merged
  upstream.

Thanks for the big effort you put into this. Much appreciated.

(In reply to comment #19)
> *hrm...
> It's been in portage for a few days now:
> *lame-3.98.2 (23 Sep 2008)

hah, i must have sync'ed just before it went in, sorry about the noise ;)