Bug 166127 - stdafx.h defines DWORD as 64-bit, breaks everything
stdafx.h defines DWORD as 64-bit, breaks everything
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: libmodplug (Show other bugs)
4
x86_64 Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2005-08-17 01:34 EDT by Adam Goode
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version: 0.7-3
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-08-26 01:44:46 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Fix for x86_64 types (850 bytes, patch)
2005-08-17 01:36 EDT, Adam Goode
no flags Details | Diff

  None (edit)
Description Adam Goode 2005-08-17 01:34:15 EDT
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.10) Gecko/20050720 Fedora/1.0.6-1.1.fc4 Firefox/1.0.6

Description of problem:
When playing a file either with xmms-modplug or modplugplay on x86_64, the file is totally jumbled and the wrong length.

Version-Release number of selected component (if applicable):
libmodplug-0.7-2

How reproducible:
Always

Steps to Reproduce:
modplugplay 2ND_PM.S3M

Actual Results:  Jumbled sounds, plays for a few seconds total.

Expected Results:  Awesome sound for a few minutes.

Additional info:
Comment 1 Adam Goode 2005-08-17 01:36:14 EDT
Created attachment 117821 [details]
Fix for x86_64 types
Comment 2 Ville Skyttä 2005-08-17 03:44:14 EDT
Thanks, I see upstream has taken care of this in CVS [1], but apparently only 
for x86_64, leaving ia64 and ppc64 possibly still broken, whereas your patch 
looks more generic.  I've contacted upstream, will wait a bit for their 
comments. 
 
[1] 
http://cvs.sourceforge.net/viewcvs.py/modplug-xmms/libmodplug/src/libmodplug/stdafx.h?r1=1.1&r2=1.2 
Comment 3 Adam Goode 2005-08-17 12:44:49 EDT
Yeah, I'm not sure though if I should be using u_int32_t or uint32_t. The first
is BSD, the second C99. u_int32_t worked without adding any new headers though,
so maybe that's the one to use?
Comment 4 Ville Skyttä 2005-08-22 12:34:43 EDT
Dunno, but an ISO standard sounds somewhat better than "BSD" to me. 
 
Anyway, there are some differences between the types changed by your patch 
versus the upstream one, could you check them?  Currently, I'm inclined to 
include the upstream fix, but test for _LP64 instead of __x86_64__.  Thoughts?  
Could you verify if that works? 
Comment 5 Adam Goode 2005-08-22 23:12:02 EDT
Upstream fix works, with _LP64 in place of __x86_64__.

I'm not sure why they are doing "typedef unsigned int ULONG" instead of "typedef
unsigned long ULONG". Maybe it's important that ULONG == DWORD? I'm not sure
this is even true always on Windows, where all these types come from. But I
guess it limits all types to 32-bit, and seems to work.
Comment 6 Ralf Corsepius 2005-08-23 20:21:03 EDT
(In reply to comment #4)
> Dunno, but an ISO standard sounds somewhat better than "BSD" to me. 
u_int_* types are non-portable, proprietary BSD types. 
Even BSD is gradually on the move to switch to their POSIX counterparts (uint_*).

> Anyway, there are some differences between the types changed by your patch 
> versus the upstream one, could you check them?  Currently, I'm inclined to 
> include the upstream fix, but test for _LP64 instead of __x86_64__.  Thoughts?  
_LP64 is non-POSIX compliant.

__x86_64__ is the official POSIX compilant define provided by GCC to identify
the x86_64 architecture.

> Could you verify if that works? 
Comment 7 Ville Skyttä 2005-08-24 02:10:52 EDT
The point was not to identify x86_64 in particular, but any 64-bit 
architecture that would need the fix.  If you know how to do that in a more 
standard compliant manner than _LP64, I would be glad to hear about it. 
 
In the meantime, I've committed the _LP64 approach and asked for builds; FC-3 
completed (and was already published, d'oh!), but unfortunately FC-4 and devel 
got stuck in the build system due to (unrelated) PPC troubles.  Keeping this 
report open until they're finished. 
Comment 8 Ralf Corsepius 2005-08-24 03:10:14 EDT
(In reply to comment #7)
> The point was not to identify x86_64 in particular, but any 64-bit 
> architecture that would need the fix.  If you know how to do that in a more 
> standard compliant manner than _LP64, I would be glad to hear about it. 
Not quite. Your actual problem is selecting appropriate types for fixed size types.

The POSIX/C99/IEEE compliant solution to this problem is using stdint.h.
cf. http://www.opengroup.org/onlinepubs/009695399/basedefs/stdint.h.html

or to mimic stdint.h as part of the application.
cf.
http://sourceware.org/cgi-bin/cvsweb.cgi/src/newlib/libc/sys/rtems/include/stdint.h?cvsroot=src

> In the meantime, I've committed the _LP64 approach and asked for builds
Well, franky speaking, if I were upstream maintainer of this package, I would
reject your patch.
Comment 9 Ville Skyttä 2005-08-24 10:57:25 EDT
The patch I've applied in this package is _from upstream_ (see comment 2), 
just with s/__x86_64__/_LP64/ for the stated reasons.  And FWIW, Debian uses 
it too (sans the replacement).  If you have issues with the current 
implementation, I suggest contacting upstream (I did a week ago, no reply). 
Comment 10 Ralf Corsepius 2005-08-24 11:42:19 EDT
(In reply to comment #9)
> The patch I've applied in this package is _from upstream_ (see comment 2), 
> just with s/__x86_64__/_LP64/ for the stated reasons.  And FWIW, Debian uses 
> it too (sans the replacement).

This doesn't mean anything.

>  If you have issues with the current 
> implementation,
Well, let me put it this way: The patch is not a fix, it is non-portable hack,
only working s by random accident on some selected subset of targets.

> I suggest contacting upstream (I did a week ago, no reply). 
Pardon, but in this case I am not sufficiently interested. If upstreams wants to
shoot themselves into the foot, so be it or we should consider to abandon the
package for immatureness. ;)
Comment 11 Adam Goode 2005-08-24 22:18:00 EDT
I'd say let's forget the __x86_64__ and _LP64 and go with stdint.h. It's
supported back to at least the version of GCC in RedHat 9. Just use my original
patch, take out the extra underscores, and add #include <stdint.h>.
Comment 12 Ville Skyttä 2005-08-26 01:44:46 EDT
The previous change was already built and entered the repository and the  
fallout is being cleaned up in dependent packages on x86_64, so let's revisit  
this later if something's actually broken in the binaries. 
 
If someone has interest in getting this fixed more cleanly and for real, I'd 
suggest feeding patches upstream. 

Note You need to log in before you can comment on or make changes to this bug.