Bug 14594 - xf86-video-ati 6.8.0 uses #pragma pack incorrectly--server fails to load
Summary: xf86-video-ati 6.8.0 uses #pragma pack incorrectly--server fails to load
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Driver/Radeon (show other bugs)
Version: unspecified
Hardware: x86 (IA32) NetBSD
: medium critical
Assignee: xf86-video-ati maintainers
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
: 14770 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-02-20 21:24 UTC by Blair Sadewitz
Modified: 2008-03-20 14:57 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
remove -DFGL_LINUX from src/Makefile.am (570 bytes, patch)
2008-02-21 06:07 UTC, Blair Sadewitz
no flags Details | Splinter Review
autoconf patch (1.25 KB, patch)
2008-03-03 07:23 UTC, Alex Deucher
no flags Details | Splinter Review

Description Blair Sadewitz 2008-02-20 21:24:21 UTC
The xf86-video-ati 6.8.0 driver is configured to build with -DFGL_LINUX.  This results in incorrect use of #pragma pack for many platforms.  The test platform in this case is NetBSD/i386 4.99.54.

Symptomatology:

The server starts, but never makes it past a blank screen--the characteristic "flicker" I noticed while [presumably] modes are being probed [for RandR] does not occur.  While the system does not hang, I nevertheless cannot switch VTs.  Thankfully, I had no problem shutting down the system properly via the ACPI power button.

At first, I could think of no reason why this should be happening.  Then, I noticed that -DFGL_LINUX is defined in the default CFLAGS--irrespective of platform.  This [I believe] resulted in incorrect alignment of structure members in the AtomBIOS headers.

Upon removing this definition and building the code with my platform's defaults, the driver worked well (and thanks for the r300 acceleration, I'm very happy to finally have it).

While I am aware that developers can only test software on machines they use, I kindly ask that in the future that at least things as obvious as a linux "FireGL specific" uses of #pragma pack are employed less capriciously.  The NetBSD project has a very competent developer who, if given commit access, would be happy to assist in [coordinating and conducting] testing on NetBSD's many supported architectures (and pkgsrc's many supported platforms) to ease the burden on other developers.  Moreover, I would think that testing Xorg code on many additional platforms could only result in more robust code.

Best Regards,

--Blair Sadewitz
<bjs@NetBSD.org>
Comment 1 Alex Deucher 2008-02-20 21:51:02 UTC
This is probably a remnant from AMD where the atom parser came from.  Please attach your patch and I'll take a look.
Comment 2 Blair Sadewitz 2008-02-21 06:07:41 UTC
Created attachment 14481 [details] [review]
remove -DFGL_LINUX from src/Makefile.am
Comment 3 Blair Sadewitz 2008-02-21 06:21:54 UTC
The section of code directly influenced by this is located around line 57 in
src/AtomBios/includes/CD_Structs.h.

//#include "atombios.h"
#if (PARSER_TYPE==DRIVER_TYPE_PARSER)
#ifdef FGL_LINUX
#pragma pack(push,1)
#else
#pragma pack(push)
#pragma pack(1)
#endif
#endif

I've never used #pragma pack myself, so if I am mistaken in my reasoning,
please let me know.  I can tell you, however, that not defining FGL_LINUX
absolutely fixed the issue.

If you need any further information, etc., please let me know.

Regards,

--Blair
Comment 4 Michel Dänzer 2008-02-21 06:27:43 UTC
(In reply to comment #2)
> Created an attachment (id=14481) [details]
> remove -DFGL_LINUX from src/Makefile.am

NACK - you expect developers on Linux to think of any potential implications of their changes on other OSs, but you didn't even bother looking up the change which added the flag with something like git-blame? Not defining FGL_LINUX causes build warnings on Linux. It looks like the code currently guarded by it needs to be diversified - as Alex pointed out, the ATOM BIOS code was previously only used on Windows and Linux.
Comment 5 Blair Sadewitz 2008-02-21 07:53:45 UTC
I did not mean that I wanted you to *remove this from the source*!

I was merely reporting what I did to make it work on my system.  Why would I want that to be removed?  It surely has an application somewhere ... just not everywhere.
Comment 6 Michel Dänzer 2008-02-21 07:59:44 UTC
(In reply to comment #5)
> I did not mean that I wanted you to *remove this from the source*!

Okay, sorry I misunderstood - I thought this was being submitted for inclusion.
Comment 7 Alex Deucher 2008-03-03 06:14:15 UTC
*** Bug 14770 has been marked as a duplicate of this bug. ***
Comment 8 Alex Deucher 2008-03-03 07:23:17 UTC
Created attachment 14789 [details] [review]
autoconf patch

Does this patch work for you?
Comment 9 Blair Sadewitz 2008-03-03 08:53:35 UTC
Yes, it does; thanks.

--Blair
Comment 10 Alex Deucher 2008-03-03 09:04:02 UTC
committed: 74eb981287d76836327830bd51272f605a07e0cc
Comment 11 Owain Ainsworth 2008-03-03 11:22:38 UTC
Unfortunately this fix doesn't fix the issue for me (my bug was marked as a dupe of this one). While it LOOKS correct, with autoconf 2.5.9 FGL_LINUX is still defined after rerunning auto{conf,make} and building.
Comment 12 Alex Deucher 2008-03-03 11:40:45 UTC
(In reply to comment #11)
> Unfortunately this fix doesn't fix the issue for me (my bug was marked as a
> dupe of this one). While it LOOKS correct, with autoconf 2.5.9 FGL_LINUX is
> still defined after rerunning auto{conf,make} and building.
> 

I guess $host_os isn't matching properly on your platform?  Any chance you could find out what autoconf setup is needed in your case?  I'm not really an autoconf expert.
Comment 13 Blair Sadewitz 2008-03-03 19:53:23 UTC
Modern NetBSD platforms usually have a tuple
like:

i386--netbsdelf (note the null second field)

for the GNU platform, so something like

*-netbsd*)

is probably what should be used.

Have you received any feedback from e.g. Solaris users about this
issue?  I'm wondering what other platforms might be affected ...

Regards,

--Blair
Comment 14 Alex Deucher 2008-03-04 06:38:34 UTC
(In reply to comment #13)
> Have you received any feedback from e.g. Solaris users about this
> issue?  I'm wondering what other platforms might be affected ...

I had a crash reported on solaris, which thinking back, may be the same issue.  I'll follow up with that user as well.
Comment 15 Owain Ainsworth 2008-03-04 09:02:46 UTC
Aparrently $host_os expands to openbsd4.3 on this version (current, 4.3 beta) of my os.

I'm stumped why this autoconf macro isn't triggering. I'll keep looking into it.
Comment 16 Blair Sadewitz 2008-03-05 09:52:14 UTC
Perhaps a better approach would be to figure out which operating
systems _DO_ need FGL_LINUX to be defined
and instead not define it by default?

--Blair
Comment 17 Alex Deucher 2008-03-06 07:55:20 UTC
I talked to a Solaris developer and it's needed there as well and he had some other suggestions for how to do it.  I'll try and get an improved patch committed soon.
Comment 18 Alex Deucher 2008-03-06 10:36:51 UTC
should be fixed in commit 766f464dfdfccadef23e4232f2bce5db22195513
Comment 19 Owain Ainsworth 2008-03-06 11:16:47 UTC
Works perfectly for me. Thank you!
Comment 20 Alex Deucher 2008-03-20 14:57:13 UTC
If this is still an issue for anyone, please reopen.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.