[PATCH] fix compilation on intel mac
Hi Guys, Here is my first attempt at a intel mac patch for mplayer. Currently libavcodec and mp3lib are still not compilling. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
Hi, On Wed, Mar 22, 2006 at 07:58:33AM -0500, Nicolas Plourde wrote:
Hi Guys, Here is my first attempt at a intel mac patch for mplayer. Currently libavcodec and mp3lib are still not compilling.
Do we already have inconsistency day or what is the reason for these variations? *g*:
+#ifdef MACOSX + ".align 4 \n\t" +#elif ".balign 16 \n\t" +#endif [...] - ".balign 16 \n\t"\ + ASMALIGN16\
Greetings, Reimar Döffinger
On 22-Mar-06, at 9:06 AM, Reimar Döffinger wrote:
Hi, On Wed, Mar 22, 2006 at 07:58:33AM -0500, Nicolas Plourde wrote:
Hi Guys, Here is my first attempt at a intel mac patch for mplayer. Currently libavcodec and mp3lib are still not compilling.
Do we already have inconsistency day or what is the reason for these variations? *g*:
+#ifdef MACOSX + ".align 4 \n\t" +#elif ".balign 16 \n\t" +#endif [...] - ".balign 16 \n\t"\ + ASMALIGN16\
I used ASMALIGN because it was inside a macro already. I can definitely use this macro in all the code. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
Hi, On Wed, Mar 22, 2006 at 08:11:02PM -0500, Nicolas Plourde wrote:
I used ASMALIGN because it was inside a macro already. I can definitely use this macro in all the code.
That would be preferable IMHO. There also was a patch on ffmpeg-dev about this, no idea if it was applied, but might be good it MPlayer and ffmpeg used the same construct... Greetings, Reimar Döffinger
On 23-Mar-06, at 5:23 AM, Reimar Döffinger wrote:
That would be preferable IMHO. There also was a patch on ffmpeg-dev about this, no idea if it was applied, but might be good it MPlayer and ffmpeg used the same construct...
Here is the fixed patch using ASMALIGN16 macro. You need to disable mp3lib and patch ffmpeg. Is this ok for checkin ? ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Tue, Apr 04, 2006 at 07:35:54AM -0400, Nicolas Plourde wrote:
On 23-Mar-06, at 5:23 AM, Reimar Döffinger wrote:
That would be preferable IMHO. There also was a patch on ffmpeg-dev about this, no idea if it was applied, but might be good it MPlayer and ffmpeg used the same construct...
Here is the fixed patch using ASMALIGN16 macro. You need to disable mp3lib and patch ffmpeg. Is this ok for checkin ?
This belongs on ffmpeg-devel. Read the archives first, there were a few patches for Intel Macs, IIRC they were similar to yours but rejected. But check it out yourself, I did not look at them in close detail. Diego
On 4-Apr-06, at 4:41 PM, Diego Biurrun wrote:
This belongs on ffmpeg-devel. Read the archives first, there were a few patches for Intel Macs, IIRC they were similar to yours but rejected. But check it out yourself, I did not look at them in close detail.
This is why there is to patch. one for mplayer specific stuff and the other for ffmpeg comming from vlc. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Wed, 22 Mar 2006 07:58:33 -0500, Nicolas Plourde <nicolas.plourde@gmail.com> said:
Hi Guys, Here is my first attempt at a intel mac patch for mplayer. Currently libavcodec and mp3lib are still not compilling.
Index: configure =================================================================== RCS file: /cvsroot/mplayer/main/configure,v retrieving revision 1.1149 diff -u -r1.1149 configure --- configure 21 Mar 2006 05:36:09 -0000 1.1149 +++ configure 22 Mar 2006 12:49:41 -0000 @@ -7028,7 +7028,11 @@ echores "$_crash_debug"
if darwin ; then + if x86 ; then + CFLAGS="$CFLAGS -DSYS_DARWIN" + else CFLAGS="$CFLAGS -mdynamic-no-pic -falign-loops=16 -DSYS_DARWIN" + fi fi
I wonder, if -falign-loops=16 is not wanted/needed, why all the effort to make .balign/.align work?
Index: mangle.h =================================================================== RCS file: /cvsroot/mplayer/main/mangle.h,v retrieving revision 1.6 diff -u -r1.6 mangle.h --- mangle.h 30 Mar 2003 20:11:05 -0000 1.6 +++ mangle.h 22 Mar 2006 12:49:41 -0000 @@ -9,7 +9,7 @@
/* Feel free to add more to the list, eg. a.out IMO */ #if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) || defined(__APPLE__) && !defined(__ELF__))
I think this should be (defined(__OpenBSD__) && !defined(__ELF__)) || defined(__APPLE__) otherwise it will be true for all __OpenBSD__ systems (and I assume Apple is never ELF). __Martin
On Wed, Mar 22, 2006 at 07:58:33AM -0500, Nicolas Plourde wrote:
+#ifdef MACOSX + ".align 4\n\t" +#elif ".balign 16\n\t" +#endif "1: \n\t" "movaps (%0, %%"REG_S"), %%xmm0 \n\t" "movaps 16(%0, %%"REG_S"), %%xmm1\n\t" @@ -714,7 +718,11 @@ "movlps %1, %%xmm7 \n\t" "shufps $0x00, %%xmm7, %%xmm7 \n\t" "mov $-1024, %%"REG_S" \n\t" +#ifdef MACOSX + ".align 4\n\t" +#elif ".balign 16\n\t" +#endif
Repeating this nonsense over and over is absolutely incorrect. I'm slightly against the patch in general, but if you want to do something like this, you must: 1. Test in configure to see if the assembler supports .balign. 2. #define a macro ALIGN_16_ASM to ".align 4" or ".balign 16" depending on the results of the test from 1. 3. Replace all occurrances of ".balign 16" with ALIGN_16_ASM. Both OS-specific ifdefs (rather than feature-specific ones) and repeated redundant ifdefs are disgusting and unacceptable from a code maintainence standpoint. Rich
On 4-Apr-06, at 5:45 PM, Rich Felker wrote:
On Wed, Mar 22, 2006 at 07:58:33AM -0500, Nicolas Plourde wrote:
+#ifdef MACOSX + ".align 4\n\t" +#elif ".balign 16\n\t" +#endif "1: \n\t" "movaps (%0, %%"REG_S"), %%xmm0 \n\t" "movaps 16(%0, %%"REG_S"), %%xmm1\n\t" @@ -714,7 +718,11 @@ "movlps %1, %%xmm7 \n\t" "shufps $0x00, %%xmm7, %%xmm7 \n\t" "mov $-1024, %%"REG_S" \n\t" +#ifdef MACOSX + ".align 4\n\t" +#elif ".balign 16\n\t" +#endif
Repeating this nonsense over and over is absolutely incorrect. I'm slightly against the patch in general, but if you want to do something like this, you must:
1. Test in configure to see if the assembler supports .balign. 2. #define a macro ALIGN_16_ASM to ".align 4" or ".balign 16" depending on the results of the test from 1. 3. Replace all occurrances of ".balign 16" with ALIGN_16_ASM.
Both OS-specific ifdefs (rather than feature-specific ones) and repeated redundant ifdefs are disgusting and unacceptable from a code maintainence standpoint.
There is no ifdef in the patch i sent this morning. I use #define ASMALIGN16. Dont take into account the ffmpeg patch its not inteded to be applied has is. Im attaching the mplayer specific patch again for review. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Tue, Apr 04, 2006 at 05:49:06PM -0400, Nicolas Plourde wrote:
There is no ifdef in the patch i sent this morning. I use #define ASMALIGN16. Dont take into account the ffmpeg patch its not inteded to be applied has is. Im attaching the mplayer specific patch again for review.
You seem to be going wild with indentation in the patch, please try to be consistent in where you put those ASMALIGN statements. Diego
On 5-Apr-06, at 3:12 AM, Diego Biurrun wrote:
On Tue, Apr 04, 2006 at 05:49:06PM -0400, Nicolas Plourde wrote:
You seem to be going wild with indentation in the patch, please try to be consistent in where you put those ASMALIGN statements.
again with consistent identation. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Wed, 5 Apr 2006 07:42:30 -0400 Nicolas Plourde <nicolas.plourde@gmail.com> wrote:
On 5-Apr-06, at 3:12 AM, Diego Biurrun wrote:
On Tue, Apr 04, 2006 at 05:49:06PM -0400, Nicolas Plourde wrote:
You seem to be going wild with indentation in the patch, please try to be consistent in where you put those ASMALIGN statements.
again with consistent identation.
At least the second occurence of ASMALIGN16 in downmix.c is not properly aligned (4 spaces + 1 tab vs. 1 tab). (and there are more of them). Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?) Aurel
On Wed, Apr 05, 2006 at 01:50:20PM +0200, Aurelien Jacobs wrote:
Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?)
I'm all for it. Are you volunteering to do the work? Diego
On Wed, Apr 05, 2006 at 11:51:28PM +0200, Diego Biurrun wrote:
On Wed, Apr 05, 2006 at 01:50:20PM +0200, Aurelien Jacobs wrote:
Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?)
I'm all for it. Are you volunteering to do the work?
I'm against it in the files I maintain, which use all tabs and no spaces, quite intentionally. Rich
Rich Felker wrote:
On Wed, Apr 05, 2006 at 11:51:28PM +0200, Diego Biurrun wrote:
On Wed, Apr 05, 2006 at 01:50:20PM +0200, Aurelien Jacobs wrote:
Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?)
I'm all for it. Are you volunteering to do the work?
I'm against it in the files I maintain, which use all tabs and no spaces, quite intentionally.
Rich
same for me
On Wed, Apr 05, 2006 at 07:00:13PM -0400, Rich Felker wrote:
On Wed, Apr 05, 2006 at 11:51:28PM +0200, Diego Biurrun wrote:
On Wed, Apr 05, 2006 at 01:50:20PM +0200, Aurelien Jacobs wrote:
Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?)
I'm all for it. Are you volunteering to do the work?
I'm against it in the files I maintain, which use all tabs and no spaces, quite intentionally.
Look closer, you have a mix of tabs and spaces at least in pullup.c. Diego
On Sat, Apr 15, 2006 at 01:17:52AM +0200, Diego Biurrun wrote:
On Wed, Apr 05, 2006 at 07:00:13PM -0400, Rich Felker wrote:
On Wed, Apr 05, 2006 at 11:51:28PM +0200, Diego Biurrun wrote:
On Wed, Apr 05, 2006 at 01:50:20PM +0200, Aurelien Jacobs wrote:
Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?)
I'm all for it. Are you volunteering to do the work?
I'm against it in the files I maintain, which use all tabs and no spaces, quite intentionally.
Look closer, you have a mix of tabs and spaces at least in pullup.c.
There is one line that includes both tabs and spaces and it's very intentional. The tabs indent to the same level as the previous line, and then the spaces provide the additional alignment to where I want the text to align to in the previous line. Note that this will look correct with _any_ tab width setting. That's entirely different from what's meant by "mixing tabs and spaces". Rich
Hi, On Wed, Apr 05, 2006 at 11:51:28PM +0200, Diego Biurrun wrote:
On Wed, Apr 05, 2006 at 01:50:20PM +0200, Aurelien Jacobs wrote:
Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?)
I'm all for it. Are you volunteering to do the work?
I agree for the files I maintain, though I probably already got rid of most *g* But in this case, is there a chance to port the commit script from ffmpeg so it would do the tabs-check for any files that have been "converted"? Greetings, Reimar Döffinger
Now with proper identation. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Thu, 6 Apr 2006 20:37:32 -0400, Nicolas Plourde <nicolas.plourde@gmail.com> said:
--- mangle.h 30 Mar 2003 20:11:05 -0000 1.6 +++ mangle.h 4 Apr 2006 11:28:04 -0000 @@ -9,7 +9,7 @@
/* Feel free to add more to the list, eg. a.out IMO */ #if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) || defined(__APPLE__) && !defined(__ELF__))
This looks wrong to me, since it will break not-ELF detection on OpenBSD. Did you mean this? #if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) && !defined(__ELF__)) || defined(__APPLE__) __Martin
On Sat, Apr 08, 2006 at 11:59:59PM +0100, Martin Simmons wrote:
On Thu, 6 Apr 2006 20:37:32 -0400, Nicolas Plourde <nicolas.plourde@gmail.com> said:
--- mangle.h 30 Mar 2003 20:11:05 -0000 1.6 +++ mangle.h 4 Apr 2006 11:28:04 -0000 @@ -9,7 +9,7 @@
/* Feel free to add more to the list, eg. a.out IMO */ #if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) || defined(__APPLE__) && !defined(__ELF__))
This looks wrong to me, since it will break not-ELF detection on OpenBSD.
Did you mean this?
#if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) && !defined(__ELF__)) || defined(__APPLE__)
I think he meant to remove all this CRAP and write a proper test for symbol mangling in configure... :) Rich
On 9-Apr-06, at 1:20 AM, Rich Felker wrote:
On Sat, Apr 08, 2006 at 11:59:59PM +0100, Martin Simmons wrote:
> On Thu, 6 Apr 2006 20:37:32 -0400, Nicolas Plourde > <nicolas.plourde@gmail.com> said:
--- mangle.h 30 Mar 2003 20:11:05 -0000 1.6 +++ mangle.h 4 Apr 2006 11:28:04 -0000 @@ -9,7 +9,7 @@
/* Feel free to add more to the list, eg. a.out IMO */ #if defined(__CYGWIN__) || defined(__MINGW32__) || defined (__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) || defined(__APPLE__) && !defined (__ELF__))
This looks wrong to me, since it will break not-ELF detection on OpenBSD.
Did you mean this?
#if defined(__CYGWIN__) || defined(__MINGW32__) || defined (__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) && !defined(__ELF__)) || defined(__APPLE__)
I think he meant to remove all this CRAP and write a proper test for symbol mangling in configure... :)
Rich
I really meant the first version and i dont see how its breaking OpenBSD. And no i will not write a proper test in configure but feel free to do it if you have the time. Now anyone have any real concern about this patch? If not i will submit it. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Sun, 9 Apr 2006 08:28:34 -0400, Nicolas Plourde said:
On 9-Apr-06, at 1:20 AM, Rich Felker wrote:
On Sat, Apr 08, 2006 at 11:59:59PM +0100, Martin Simmons wrote:
>> On Thu, 6 Apr 2006 20:37:32 -0400, Nicolas Plourde >> <nicolas.plourde@gmail.com> said:
--- mangle.h 30 Mar 2003 20:11:05 -0000 1.6 +++ mangle.h 4 Apr 2006 11:28:04 -0000 @@ -9,7 +9,7 @@
/* Feel free to add more to the list, eg. a.out IMO */ #if defined(__CYGWIN__) || defined(__MINGW32__) || defined (__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) || defined(__APPLE__) && !defined (__ELF__))
This looks wrong to me, since it will break not-ELF detection on OpenBSD.
Did you mean this?
#if defined(__CYGWIN__) || defined(__MINGW32__) || defined (__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + (defined(__OpenBSD__) && !defined(__ELF__)) || defined(__APPLE__)
I think he meant to remove all this CRAP and write a proper test for symbol mangling in configure... :)
Rich
I really meant the first version and i dont see how its breaking OpenBSD.
The problem is that your version is equivalent to this: #if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + defined(__OpenBSD__) || \ + (defined(__APPLE__) && !defined(__ELF__)) so it will change OpenBSD with ELF to add the underscore. AFAIK, ELF never has an underscore and also Apple never use ELF. __Martin
On Sun, Apr 09, 2006 at 08:45:28PM +0100, Martin Simmons wrote:
#if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + defined(__OpenBSD__) || \ + (defined(__APPLE__) && !defined(__ELF__))
so it will change OpenBSD with ELF to add the underscore. AFAIK, ELF never has an underscore and also Apple never use ELF.
Wow, did Apple do something right for once??? (rejecting elf) Rich
On 9-Apr-06, at 3:45 PM, Martin Simmons wrote:
The problem is that your version is equivalent to this:
#if defined(__CYGWIN__) || defined(__MINGW32__) || defined (__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + defined(__OpenBSD__) || \ + (defined(__APPLE__) && !defined(__ELF__))
so it will change OpenBSD with ELF to add the underscore. AFAIK, ELF never has an underscore and also Apple never use ELF.
Humm... you are right. I fixing it. Anything else? ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Apr 10, 2006, at 6:44 AM, Nicolas Plourde wrote:
On 9-Apr-06, at 3:45 PM, Martin Simmons wrote:
The problem is that your version is equivalent to this:
#if defined(__CYGWIN__) || defined(__MINGW32__) || defined (__OS2__) || \ - (defined(__OpenBSD__) && !defined(__ELF__)) + defined(__OpenBSD__) || \ + (defined(__APPLE__) && !defined(__ELF__))
so it will change OpenBSD with ELF to add the underscore. AFAIK, ELF never has an underscore and also Apple never use ELF.
Humm... you are right. I fixing it. Anything else?
Hi Just joined the listed yesterday. Found your patch in the archives. Looking at a patch submitted for ffmpeg, is there any reason for not replacing .align with .p2align instead of doing symbol mangling? Looking forward to getting things working so I can resume putting up mplayerosx builds <http://www.haque.net/software/mplayerosx/cvs/>. -- ===================================================================== Mohammad A. Haque http://www.haque.net/ mhaque@haque.net "Alcohol and calculus don't mix. Don't drink and derive." --Unknown =====================================================================
On Mon, Apr 10, 2006 at 08:49:26AM -0400, Mohammad A. Haque wrote:
Looking at a patch submitted for ffmpeg, is there any reason for not replacing .align with .p2align instead of doing symbol mangling?
Huh? This is not a symbol... The two things you asked about are unrelated; there's no way to "do one instead of the other". Rich
On Apr 10, 2006, at 12:37 PM, Rich Felker wrote:
On Mon, Apr 10, 2006 at 08:49:26AM -0400, Mohammad A. Haque wrote:
Looking at a patch submitted for ffmpeg, is there any reason for not replacing .align with .p2align instead of doing symbol mangling?
Huh? This is not a symbol... The two things you asked about are unrelated; there's no way to "do one instead of the other".
Err sorry. Not really a hardcore programmer. I was just wondering if the following is correct (based on what I read if I'm understanding it correctly): .balign 16 on systems other than darwin == .align 4 on darwin .p2align 4 on systems other than darwin == .p2align 4 on darwin Why not just .p2align instead of the #defines? -- ===================================================================== Mohammad A. Haque http://www.haque.net/ mhaque@haque.net "Alcohol and calculus don't mix. Don't drink and derive." --Unknown =====================================================================
On 10-Apr-06, at 3:47 PM, Mohammad A. Haque wrote:
Err sorry. Not really a hardcore programmer.
I was just wondering if the following is correct (based on what I read if I'm understanding it correctly):
.balign 16 on systems other than darwin == .align 4 on darwin .p2align 4 on systems other than darwin == .p2align 4 on darwin
Why not just .p2align instead of the #defines?
I dont have have all the x86 platform that mplayer support available for testing, So Im not to tempted to try changing stuff that already work. ==== Nicolas Plourde <nicolas.plourde@gmail.com>
On Thu, Apr 06, 2006 at 12:25:33PM +0200, Reimar D?ffinger wrote:
On Wed, Apr 05, 2006 at 11:51:28PM +0200, Diego Biurrun wrote:
On Wed, Apr 05, 2006 at 01:50:20PM +0200, Aurelien Jacobs wrote:
Maybe it would be time to remove all those tabs in some parts of MPlayer ? (at least in files whose maintainers agree ?)
I'm all for it. Are you volunteering to do the work?
I agree for the files I maintain, though I probably already got rid of most *g* But in this case, is there a chance to port the commit script from ffmpeg so it would do the tabs-check for any files that have been "converted"?
Yes, that would be trivial. Diego
participants (8)
-
Aurelien Jacobs -
Diego Biurrun -
Martin Simmons -
Mohammad A. Haque -
Nico Sabbi -
Nicolas Plourde -
Reimar Döffinger -
Rich Felker