Red Hat Bugzilla – Bug 166127
stdafx.h defines DWORD as 64-bit, breaks everything
Last modified: 2007-11-30 17:11:11 EST
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):
Steps to Reproduce:
Actual Results: Jumbled sounds, plays for a few seconds total.
Expected Results: Awesome sound for a few minutes.
Created attachment 117821 [details]
Fix for x86_64 types
Thanks, I see upstream has taken care of this in CVS , 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
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?
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?
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.
(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?
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.
(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.
or to mimic stdint.h as part of the application.
> 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.
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).
(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
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. ;)
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>.
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.