gfx_crit_sect_nesting bugs & no-asm in windows & whatever
orz

Multiple things in this post:

1. Could anyone cluefull about Allegro internals take a glance at this thread:
http://www.allegro.cc/forums/thread/590226
and see if they have anything to add to the discussion?

2. Asm in src/win in Allegro:
Recently I've been attempting to examine Allegro code in my debugger and experiment with altered versions of Allegro. The best way I could think of to do this was to create a new project using my IDE (MSVC 7.1) and copy all Allegro source and headers into it. This required disabling Allegro's asm code since MSVC can't deal with AT&T syntax. However, this didn't work the windows-specific bitmap locking code, as there doesn't seem to be any implementation of it except in AT&T asm. I rewrote it in C (the functions were extremely simple - generally 2 to 4 lines of C code per function) to get around that, and then all the allegro code compiled. This also changed the calling conventions, but everything seemed to work so I think the headers avoided the non-standard calling conventions the asm used automatically when asm code is disabled. Everything seems to work, at least with what little I've done in my test program so far (a few textprintfs, blits, some random callbacks and input stuff).

But anyway, the point:
Is there a better way to debug &| experiment with Allegro on Windows? A recommended way?
and
Why doesn't Allegro support no-asm or no-AT&T asm out of the box? This would allow things like an alternate build process that did not require GNU.

edit: added this code
edit2: added #includes, fixed externs, corrected footnotes at the end
edit5: fixed code bug (missing ! operator in gfx_directx_unwrite_bank_win)
edit6&7: changed unsigned long ints to uintptr_ts. added #ifdef, removed a #include
source code to replace asmlock.s:

1#include "allegro.h"
2#include "wddraw.h"
3 
4#if defined ALLEGRO_NO_ASM || defined ALLEGRO_USE_C
5 
6extern char *wd_dirty_lines; /* used in WRITE_BANK() */
7extern void (*update_window) (RECT *rect); /* window updater */
8extern char *gdi_dirty_lines; /* used in WRITE_BANK() */
9void gfx_gdi_autolock (struct BITMAP* bmp);
10void gfx_gdi_unlock (struct BITMAP* bmp);
11void gfx_directx_autolock (struct BITMAP* bmp);
12void gfx_directx_unlock (struct BITMAP* bmp);
13 
14static void update_dirty_lines (BITMAP *bmp) {
15 int i;
16 RECT rect;
17 rect.left = 0;
18 rect.right = bmp->w;
19 for (i = 0; i < bmp->h; i++) {
20 if (wd_dirty_lines<i>) {
21 int j = i+1;
22 rect.top = i;
23 for (; wd_dirty_lines[j]; j++) {
24 }
25 rect.bottom = j;
26 update_window(&rect);
27 i = j+1;
28 }
29 }
30}
31 
32uintptr_t gfx_directx_write_bank (BITMAP *bmp, int line) {
33 if (!(bmp->id & BMP_ID_LOCKED))
34 gfx_directx_autolock(bmp);
35 return (uintptr_t) bmp->line[line];
36}
37 
38void gfx_directx_unwrite_bank (BITMAP *bmp) {
39 if (!(bmp->id & BMP_ID_AUTOLOCK)) return;
40 gfx_directx_unlock(bmp);
41 bmp->id &= ~ BMP_ID_AUTOLOCK;
42}
43 
44uintptr_t gfx_directx_write_bank_win (BITMAP *bmp, int line) {
45 wd_dirty_lines[bmp->y_ofs+line] = 1;
46 if (!(bmp->id & BMP_ID_LOCKED)) gfx_directx_autolock(bmp);
47 return (uintptr_t) bmp->line[line];
48}
49 
50void gfx_directx_unwrite_bank_win (BITMAP *bmp) {
51 if (!(bmp->id & BMP_ID_AUTOLOCK)) return;
52 gfx_directx_unlock(bmp);
53 bmp->id &= ~BMP_ID_AUTOLOCK;
54 update_dirty_lines(gfx_directx_forefront_bitmap);
55 return;
56}
57 
58void gfx_directx_unlock_win (BITMAP *bmp) {
59 gfx_directx_unlock(bmp);
60 if (!(gfx_directx_forefront_bitmap->id & BMP_ID_LOCKED))
61 update_dirty_lines(gfx_directx_forefront_bitmap);
62 return;
63}
64 
65uintptr_t gfx_gdi_write_bank (BITMAP *bmp, int line) {
66 gdi_dirty_lines[bmp->y_ofs] = 1;
67 if (!(bmp->id & BMP_ID_LOCKED)) gfx_gdi_autolock(bmp);
68 return (uintptr_t) bmp->line[line];
69}
70 
71void gfx_gdi_unwrite_bank (BITMAP *bmp) {
72 if (!(bmp->id & BMP_ID_AUTOLOCK)) return;
73 gfx_gdi_unlock(bmp);
74 bmp->id &= ~ BMP_ID_AUTOLOCK;
75}
76#endif//defined ALLEGRO_NO_ASM || defined ALLEGRO_USE_C

Additional changes required:
wddraw.h: the section commented with "bitmap locking (from wddlock.c and asmlock.s)"
prototypes need to be fixed there
wddlock.c: gfx_directx_autolock and gfx_directx_unlock should be made non-static
wgdi.c: gfx_gdi_autolock and gfx_gdi_unlock should be made non-static

edit3:
Possible additional things:
if asmlock.s is eliminated, then the ptr_directx_* and ptr_gdi_* function pointers can be removed (they were stupid anyway)
if asmlock.s is not removed, then this code needs to be compiled & linked conditionally on the no-asm option, either via an #ifdef or via the build process somehow (ie the c directory or whatever)

3. gfx_crit_sect_nesting bugs

First of all: _exit_gfx_critical is defined as

#define _exit_gfx_critical()   LeaveCriticalSection(&gfx_crit_sect); \
                               gfx_crit_sect_nesting-- 

This appears to be incorrect: a context switch can happen during the decrement of gfx_crit_sect_nesting (or alternatively, on a dual-CPU system, another thread can run concurrently with it). The new thread could decrement gfx_crit_sect_nesting after the first had read it and before it had written to it. Even if a INC instruction was used (which compilers rarely do), it could still happen on a multiCPU system. This could happen even in regular allegro programs, as the allegro thread calls _exit_gfx_critical() during WM_PAINT handling, and the main thread calls it during graphics operations.

Now we come to the convergence of #1 and #2. In the thread I linked to in #1, I suggested displaying the value of gfx_crit_sect_nesting, on the theory that maybe there was a mismatch between locking and unlocking involved. Trying my own advice, I'm now displaying gfx_crit_sect_nesting in my own test project... and it's looks very very bad. As in, it starts properly at 0, but if I wave the window around it reaches -1 after a minute or so, then -2, then -3... I'm on a dual-core computer, that may be relevant. This can't be the bug I previously mentioned (I think) - the value is too low, not too high (besides, the previous one ought to be difficult to observe). I have reproduced this on an unmodified regular static build of Allegro. I'll start trying to figure out why now... others might want to check if Allegro does the same on their computer:

1#include "allegro.h"
2#include "winalleg.h"
3 
4#include <allegro/platform/aintwin.h>
5 
6int main(int argc, char **arv) {
7 allegro_init();
8 install_keyboard();
9 set_color_depth(32);
10 if (set_gfx_mode(GFX_AUTODETECT_WINDOWED, 640, 480, 0, 0) != 0) {
11 printf("failed to set graphics mode\n");
12 exit(0);
13 }
14 BITMAP *bmp = create_video_bitmap(160, 20);
15 clear(bmp);
16 textprintf(bmp, font, 0, 0, palette_color[15], "hello2!");
17 textout_centre_ex(screen, font, "Hello, world!", SCREEN_W/2, SCREEN_H/2, makecol(255,255,255), -1);
18 blit(bmp, screen, 0, 0, 10, 10, 160, 20);
19 blit(bmp, bmp, 0, 0, 100, 10, 160, 20);
20 while (!key[KEY_ESC]) {
21 textprintf (screen, font, 20, 200,makecol(255,255,255), "%d", gfx_crit_sect_nesting);
22 blit(bmp, screen, 0, 0, 10, 10, 160, 20);
23 }
24}
25END_OF_MAIN()

Requires a static build of allegro. I grab the title bar of the window and wave it around. After about a minute of that, it usually has a counted down to -3 or so. I'm building this as a console program, but I don't think it matters.

edit4: Now I'm having a hard time getting more negative gfx_crit_sect_nesting events. I used to get about 1 every 20 seconds, but it's fallen off to about 1 every 5 minutes now. Sitting around waving the mouse around for 5 minutes is no fun. I have yet to get even a single such event if I make the decrement happen before the unlock, so perhaps I'm confused about that producing a high number instead of a low number. But, maybe not, because I didn't start trying until the rate fell.

Peter Wang

2. The C-only code originally existed so that Allegro could be ported to non-intel machines, running some flavour of Unix. Nobody ever got around to making the C-only code work on Windows as well. It would be very nice if you sent in a patch, if you've got it working.

3. _exit_gfx_critical() does look wrong. Have you tried decrementing gfx_crit_sect_nesting before leaving the critical section? [edit: looks like it worked?]

orz

2. The code works as posted there (well, in all tests so far... haven't tried GDI mode yet...). I suppose I should try to make a patch... GNU and I generally don't get along well...

3. Yeah, I think _exit_gfx_critical fixed the negative counts there. I was initially thinking that the race would be two decrements happening at once under VERY rare circumstances, producing positive counts instead of negative counts. However, on further thought, the other thread can be inside the lock at that time, so it could be incrementing, resulting in positive values. And that's a FAR more likely case, since if the 2nd thread was blocking on the condition (a common scenario if the main thread is doing graphics when a WM_PAINT was recieved) then when in unlocks the first thread is decrementing at pretty much the same time that the second thread is incrementing, making either errors in either direction possible.

edit: Also, should gfx_crit_sect_nesting be volatile? my understanding is that it should be, but there's some strange rumors around about the exact meaning of volatile. I don't think it really matters though.

Evert
On Wednesday 28 February 2007 02:35, Peter Wang said:

2. The C-only code originally existed so that Allegro could be ported to
non-intel machines, running some flavour of Unix. Nobody ever got around
to making the C-only code work on Windows as well. It would be very nice
if you sent in a patch, if you've got it working.

One of the reasons this wasn't done before now (I think it was discussed in
2005 or so, possibly earlier and later as well) is that the pure-C DLL would
not be binary compatible with the DLL with ASM calling conventions - at least
that's the argument I remember being given for it, though I've (obviously)
never tried it out. We should have fixed this for 4.2.0, but it didn't get
done for some reason.
It would be good to fix it, because it's really the only thing standing in the
way of a "pure" MSVC version of Allegro.

(PS: The post-by-mail feature seems to be broken; is it supposed to still work?)

orz

Compatibility:
I guess I hadn't really thought the compatibility issue through. But now I shall!...

Source compatibility is a given of course. But two remaining levels of compatibility exist: compatibility between asm and non-asm dlls and libs, and compatibility between older and newer dlls and libs. The latter is not an issue for 4.3.*, but important for 4.2.x. Therefore, different solutions may be appropriate for 4.3 and 4.2.

4.0 & 4.2:
Given the constraints necessary for full binary compatibility, it is impossible to implement the functions in plain C. However, it CAN be implemented seperately for each supported compiler (what, MSVC, gcc, watcom, maybe Intel-C?) with #ifdefs and inline asm or calling convention overrides applied to the function declarations. I'm capable of handling that for MSVC and gcc if necessary. I don't have watcomm or intel though. This allows a simpler build process, but is not sufficient for full compatibility with arbitrary compilers.
Other ideas: binary compatible versions must exist, but that doesn't mean new header files have to use them; instead, the code statically linked to the app or somehow quasi-inlined (ie have the function compiled as part of the users code, but still not inlined since it must be called indirectly (unless it wants to branch on bitmap type...)).

4.3:
There's a lot of options for 4.3. The simplest would be to just switch the code to C. I suspect that the current scheme has no speed advantage compared to C on MSVC and only a minute performance advantage on other compilers.
If speed is a serious concern (I'm dubious, but it could be) and ability to compile Allegro itself on no-name compilers is not, then one solution becomes to have multiple copies of the functions (probably one plain-C and one __fastcall for MSVC; I think all the other compilers can efficiently (though with ugly syntax) call MSVCs __fastcall functions) in the dll or lib, and have the headers select the best ones at the time of compilation of the app, not the lib/dll. Note that this does not prevent support for new compilers being added partway through 4.4. Nor does it prevent arbitrary C compilers from linking with Allegro, they're only prevent from building it.
If the ability to compile Allegro on any standard C compiler is the priority, then I think there is no better solution than just plain C.

Peter Wang

Don't worry about ABI incompatibility between the asm and C versions. We'd only guarantee compatibility for the asm version anyway, as we do for x86 linux. We might want to make the C version result in a different DLL name, just so people don't inadvertently distribute incompatible DLLs.

For 4.3: yes, we're planning to switch to C anyway. I think we can do it as soon as we have the patch for the Windows port ;)

orz
Quote:

Don't worry about ABI incompatibility between the asm and C versions. We'd only guarantee compatibility for the asm version anyway, as we do for x86 linux. We might want to make the C version result in a different DLL name, just so people don't inadvertently distribute incompatible DLLs.

Well, that makes the code a lot easier for 4.2.1.

Quote:

For 4.3: yes, we're planning to switch to C anyway. I think we can do it as soon as we have the patch for the Windows port

Well... my patch-making skills are less 1337 than my race-condition-finding skills. I have attempted to make a patch. It gives me errors when I try to apply it though. I probably screwed something up somehow, but possibly I'm screwing up applying the patch instead of making it. The text in the patch file looks right to me.

Anyway, I've attached a zip to this post (asmlock.zip). The zip contains my non-working patch file, and my modified versions of every source file I modified, including the new source file I created. The zip was made using pkzip 4.0, and does not contain any directory information. The modifications are to 4.2.1 code. I'll check up on the 4.3.0 code to see if I had to do anything else different there, maybe post another zip for that.

Also, I'm not sure, but client code using one of these builds MIGHT have to define the ALLEGRO_NO_ASM or ALLEGRO_USE_C symbols to get the header files to use the right bmp_write_line etc implementations.

Also, you might prefer a different name for asmlock.c, say, wc_lock.c... the current name is an oxymoron, I've been using it because it's intuitive (asmlock.c is equivalent to asmlock.s, more or less) and because it makes the two files show up next to each other in alphabetical order in my IDE.

edit: Oh yeah... I used 1-tab indentation in asmlock.c... I believe Allegro uses that gnuish standard of X spaces or X-Y spaces and 1 tab or X-Y-Z spaces and 1 tab if it's the first tab standard (not sure what the values of X,Y,&Z are) that never looks right unless your editor is set up just right. I'm not sure how to reproduce that indentation, sorry : ).

Peter Wang

The patch applies fine. In the Allegro directory, use the command patch -p1 -i /path/to/patch

You don't need to update the 4.3 code. Once this patch is committed in the 4.2 branch it should be trivial to merge the changes into the 4.3 branch. However, it requires a bit of work. I don't like the forward declarations in asmlock.c, which look like a maintenance nightmare.

Allegro uses 3 space indents, with tabs (now deprecated) expanding to multiples of 8 (as is traditional). Nothing to do with GNU.

orz
Quote:

However, it requires a bit of work. I don't like the forward declarations in asmlock.c, which look like a maintenance nightmare.

Yeah... many of the global symbols referred to only by asm code are not listed in any header file, at least for the asm code I looked at. I did not create header files for them, nor stick them into preexisting header files. The solutions that come to mind would be
1. stick them into wddraw.h in the section currently commented "bitmap locking (from wddlock.c and asmlock.s)". The comment would have to be changed to also mention wgdi.c.
2. Create new wgdi.h and possibly wddlock.h. Possibly move a few lines from wddraw section mentioned above to wddlock.h. Possibly also a asmlock.h to move the remaining things from that section of wddlock.h into. asmlock.c would need extra #includes added.

Not much effort required either way. You can do it or I can do it and attempt to make another patch file : ) Any preferences?

Quote:

Allegro uses 3 space indents, with tabs (now deprecated) expanding to multiples of 8 (as is traditional). Nothing to do with GNU.

Somehow I thought the tradition involved the first tab having a different size than the others, dunno how I came up with that idea.

Peter Wang

You do it ;) Also, add some comments.

Have you tested it with mingw? I very quickly tried to build it using a mingw cross-compiler in linux, but I think the makefiles are currently still pulling in the asm files even when I pass ALLEGRO_USE_C=1. Anyway, it shouldn't be too hard to sort out.

Curiously, ALLEGRO_USE_C support was claimed to have been added to the mingw port in 2001. But this was before the merger of ALLEGRO_USE_C and ALLEGRO_NO_ASM (which I think actually meant: no asm calling conventions for bank switching routines). So it probably used to be possible to use C drawing routines but asm bank switching convention in the mingw port. We took a step backwards there.

orz
Quote:

Any preferences?

Quote:

You do it ;) Also, add some comments.

Heh. I was also referring to which header file scheme to use, the one more like the code already there or the one more like the normal C way of doing things. But I can just flip a coin on that issue I guess.

Quote:

Have you tested it with mingw? I very quickly tried to build it using a mingw cross-compiler in linux, but I think the makefiles are currently still pulling in the asm files even when I pass ALLEGRO_USE_C=1. Anyway, it shouldn't be too hard to sort out.

MinGW isn't being nice to me at the moment; I can't even build unmodified Allegro with it atm. I can build Allegro with DJGPP, but that's not much help : ). I expect the makefiles may need some touch-up, but that should probably be done by some one other than me... /me is worse with makefiles than with patch/diff.

Thread #590303. Printed from Allegro.cc