Help improve my code! Suggestions please =]

[Moddy]

Hey guys and gals! I've just been experimenting with the winAPI - still, I've just been butchering code from tutorials to be honest. Adding my own bits and pieces. I don't know how hell I'm going to remember the sequence for WinMain! [Calling the functions in the correct order, filling the classes correctly etc..] As I know C++ I might practice with that for the WinAPI aswell, so I gain some more knowledge.

The reason I'm posting though is to ask if anyone has any suggestions for this code; It took me a litle while to do but I am LOVING learning this! Anyway, Onto the code:

Code Select Expand


;21st October 2007 - 23:55
;Moddy - Second ever MASM app. First with WinAPI.
.486
.model flat, stdcall
option casemap :none

;standard includes
include \masm32\include\windows.inc
include \masm32\include\user32.inc
include \masm32\include\kernel32.inc

includelib \masm32\lib\user32.lib
includelib \masm32\lib\kernel32.lib

;Function Prototypes
WinMain proto :DWORD, :DWORD, :DWORD, :DWORD  ;WinMain = Required Windows Main Function
ExitMsg proto :DWORD   ;My little "wannabe error-handling" function.

.data
;Windows requirements...
ClassName   db "WinClass", 0
AppName   db "Sandbox Application", 0
;Quit Messages...
QuitMsg   db "Successfully Quiting!", 0
FailMsg db "Application Failed", 0

;Uninitialised data
.data?
;hInstance stores the handle to the module to be associated with the applicated :/
hInstance   HINSTANCE ?

.code
start:
;Run GetModelHandle, Then move the return from it into hInstance. [The return of course, being in eax]
invoke GetModuleHandle, NULL
mov hInstance, eax
;Run WinMain, The Main Windows Process,
invoke WinMain, hInstance, NULL, NULL, 0
invoke ExitProcess, eax

;Obligatory WinMain function
  WinMain proc hInst:HINSTANCE, hPrevInst:HINSTANCE, CmdLine:LPSTR, CmdShow:DWORD
;3 Local Variables
local wc:WNDCLASSEX
local msg:MSG
local hwnd:HWND

;Fill up the WNDCLASSEX class with the required information
mov    wc.cbSize, SIZEOF WNDCLASSEX
mov    wc.style, CS_HREDRAW or CS_VREDRAW
mov    wc.lpfnWndProc, offset WndProc
mov    wc.cbClsExtra, NULL
mov    wc.cbWndExtra, NULL
;Use push and pop to move hInstance into wc.hInstance. REMEMBER: mov wont do memory to memory, just reg <-> mem, and reg <-> reg!
push   hInstance
pop    wc.hInstance
mov    wc.hbrBackground, COLOR_WINDOW+1
mov    wc.lpszMenuName, NULL
mov    wc.lpszClassName, offset ClassName
;Invoke LoadIcon - Used to choose the icon for the application
invoke LoadIcon, NULL, IDI_APPLICATION

mov    wc.hIcon, eax
mov    wc.hIconSm, eax
;LoadCursor - Used to choose the cursor for the application
invoke LoadCursor, NULL, IDC_ARROW

mov    wc.hCursor, eax
;CreateWindowEx - Registers a windows class for use when we call CreateWindowEx
invoke RegisterClassEx, addr wc

;Create our Window!
invoke CreateWindowEx, 0, addr ClassName, addr AppName, WS_OVERLAPPEDWINDOW or WS_VISIBLE, CW_USEDEFAULT,
                                                  CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT, NULL, NULL, hInst, NULL
  ;Move the return from CreateWindowEx [which is currently in eax] into hwnd
mov   hwnd, eax

;Create a loop which won't stop running unless someone clicks something...
.while TRUE
;Which will send a message to the app, Which GetMessage will catch...
invoke GetMessage, addr msg, NULL, 0, 0
.break .if (!eax)
;And TranslateMessage will convert into characters...
invoke TranslateMessage, addr msg
;For DispatchMessage to deal with. [i.e Send it to WndProc]
invoke DispatchMessage, addr msg
.endw

mov eax, msg.wParam
ret
WinMain endp

;This is where the messages are passed to be handed.
WndProc proc hWnd:HWND, uMsg:UINT, wParam:WPARAM, lParam:LPARAM
;If the message is WM_DESTROY then push 0, and call ExitMsg [Push 0 so ExitMsg can pop it off after its called]
.if uMsg == WM_DESTROY
push 0
invoke ExitMsg, hWnd
;DefWindowProc = The defualt message handler. So should the message NOT be WM_DESTROY - Pass it on to DefWindowProc.
.else
invoke DefWindowProc, hWnd, uMsg, wParam, lParam
ret
;Make eax 0 [xor by itself] then return.
.endif
xor eax, eax
ret
WndProc endp

;This is my *attempt* at an error handling function :P
ExitMsg proc hWnd:HWND
;Pop the argument off...
pop ebx
;If its zero then all is good!
.if (ebx)
;So we can output our messagebox with the good message, and then exit.
invoke MessageBox, hWnd, addr QuitMsg, addr QuitMsg, MB_OK 
invoke ExitProcess, 0
ret
.endif
;But if we get here, then we can't have been supplied zero. So all is NOT good.. We call MessageBox with the bad message then exit with the value that was supplied to us
invoke MessageBox, hWnd, addr FailMsg, NULL, MB_OK ;By setting the 3rd arg as NULL, It will display the default. [Default = "Error"]
invoke ExitProcess, ebx
ret
ExitMsg endp

end start


So any ideas or suggestions for making that more efficient? Or any observations/corrections?! Thanks! =]

P.S Even though I'm not posting a lot, I'm lurking and absolutely loving what I'm reading!

[MichaelW]

I'm not sure exactly what you are trying to do with the push 0 and pop ebx, but as coded the value popped into ebx will not be the zero that was pushed. This is a disassembly of your window procedure:

Code Select Expand


004010F8 55                     push    ebp
004010F9 8BEC                   mov     ebp,esp
004010FB 837D0C02               cmp     dword ptr [ebp+0Ch],2
004010FF 750C                   jnz     loc_0040110D
00401101 6A00                   push    0
00401103 FF7508                 push    dword ptr [ebp+8]
00401106 E81D000000             call    fn_00401128
0040110B EB15                   jmp     loc_00401122
0040110D                    loc_0040110D:
0040110D FF7514                 push    dword ptr [ebp+14h]
00401110 FF7510                 push    dword ptr [ebp+10h]
00401113 FF750C                 push    dword ptr [ebp+0Ch]
00401116 FF7508                 push    dword ptr [ebp+8]
00401119 E852000000             call    fn_00401170
0040111E C9                     leave
0040111F C21000                 ret     10h
00401122                    loc_00401122:
00401122 33C0                   xor     eax,eax
00401124 C9                     leave
00401125 C21000                 ret     10h

The zero is pushed before the hWnd parameter is pushed, and then the call instruction is pushing the return address before jumping to the ExitMsg procedure. The stack grows down in memory, with the push instruction subtracting 4 from esp and then copying its operand to the address pointed to by esp, so the contents of the stack at that point, relative to esp, are:

Code Select Expand


esp -> return address
esp+4 -> hWnd parameter
esp+8 -> 0

And this is a disassembly of your ExitMsg procedure:

Code Select Expand


00401128                    fn_00401128:
00401128 55                     push    ebp
00401129 8BEC                   mov     ebp,esp
0040112B 5B                     pop     ebx
0040112C 0BDB                   or      ebx,ebx
0040112E 741F                   jz      loc_0040114F
00401130 6A00                   push    0
00401132 681D304000             push    40301Dh
00401137 681D304000             push    40301Dh
0040113C FF7508                 push    dword ptr [ebp+8]
0040113F E84A000000             call    fn_0040118E
00401144 6A00                   push    0
00401146 E855000000             call    fn_004011A0
0040114B C9                     leave
0040114C C20400                 ret     4

After the push ebp and mov ebp,esp the contents of the stack, relative to ebp (or esp, the values are the same at that point), are:

Code Select Expand


esp -> preserved ebp
esp+4 -> return address
esp+8 -> hWnd parameter
esp+12 -> 0

The pop instruction copies the value at the address pointed to by esp to its operand, and then adds 4 to esp, so the pop ebx instruction is popping the ebp value that was pushed at the start of the procedure. You could alter the code to get the pushed zero from the stack, but it would make more sense to just add another parameter to the procedure and use it in the normal manner.

Also, the push 0 and pop ebx are causing stack imbalances. While this is not generally a problem in procedures with a stack frame, because esp is preserved at entry and restored before the procedure returns, in a procedure without a stack frame it would cause the procedure to return to the wrong address. See masm32\help\asmintro.hlp, The Stack.

Also, your code changes ebx without preserving the value, in code that is called from a callback, in this case the window procedure. See masm32\help\asmintro.hlp, Register Preservation Convention.

Also, .if  (ebx) returns true when ebx != 0.

[Moddy]

Ok, So I've taken your thoughts on board - and I've produced a few changes. Mainly;
a)I now have a UINT variable named "stat" to store the value.
b)The if statement now looks like "if (stat == 0)

What I am trying to do is have a function that can be called to exit the application, but depending on a certain value - it will display a messagebox as it exits. This *may* seem a little useless in the size of this application, but I was messing about and just experimenting really. As I could see the idea being quite useful.

[So I don't make this thread massive with repeating the code over and over again, you can see my changes here.]

Thanks!

[gabor]

Hi Moddy!

MichaelW has already told everything you have to deal with. I'd like to recommand (more or less a convention) to pass arguments via the eax or on the stack.
In this case eax could be sufficient since there is only 1 argument and using eax is the most general way:

Code Select Expand


...
mov eax,0
invoke ExitMsg, hWnd
...

or pass on the stack

Code Select Expand


...
invoke ExitMsg, hWnd,0
...
ExitMsg proc hWnd:HWND,dwErrorCode:DWORD
...

This 2nd solution produces a more readable code.
This is more a matter of taste. There are coders who prefer using regs (eax, ecx, edx - they don't have to be preserved across API calls) because it might be faster: 1 mov instead of 1 push and 1 pop or mov. Other point of view is to keep the code as readable and understandable as possible.
My conclusion is that both can be used depending on the purpose of the code. It has to be fast then use regs otherwise use the stack.
Anyway, you are on the right track. Keep up learnig and asking questions!  :U

...I was writing this post when you replied to MichaelW, so I add how I usually code this kind of error handling.
1. I create a table of pointers to the error strings
2. The error code is either transformed to select a pointer (thus a string) or the error code is a 1 to 1 association to a pointer/string.
3. If the error code is not 0 I display a messagebox containing the string pointed by the selected table element.

Greets, Gábor

[Moddy]

Gábor: Thanks! Your error handling idea seems to be a more powerful one - I might have to try and implement something like that.

I think I'll end up using the stack for passing on the arguments. My little "fix" a few minutes ago was done quickly as I'm in a bit of a rush to go out! But when I get back I'll have a go at changing my error handling to something a bit more efficient, Then I'll probably have a look at changing the way I'm passing the arguments.

Thanks again!

[lingo]

The same but in pure assembly without "C/C++ like assembly"

Code Select Expand


.data
        ;Windows requirements...
        ClassName    db "WinClass", 0
        AppName      db "Sandbox Application", 0
        ;Quit Messages...
        QuitMsg    db "Successfully Quiting!", 0
        FailMsg    db "Application Failed", 0
wc   dd sizeof WNDCLASSEX
     dd CS_HREDRAW or CS_VREDRAW or CS_BYTEALIGNWINDOW
     dd offset WndProc, 0, 0, 400000h, 0, 0
     dd COLOR_WINDOW +1, 0
     dd offset  ClassName, 0

;Uninitialised data
.data?
        ;hInstance stores the handle to the module to be associated with the applicated :/
        ;hInstance   HINSTANCE ?  -> it is always 400000h!
        stat  dd ?
        hWnd  dd ? 
        msg   MSG <>
.code
Start:
       push  IDI_APPLICATION
       push  0
       call  LoadIcon
       mov   wc.WNDCLASSEX.hIcon, eax
       mov   wc.WNDCLASSEX.hIconSm, eax
       push  IDC_ARROW
       push  0
       call  LoadCursor
       mov   wc.WNDCLASSEX.hCursor, eax
       push  offset wc       
       call  RegisterClassEx ; Register Main Window Class

       push  0          ; Create Main Window
       push  400000h    ; hInstance = 400000h always
       push  0
       push  0
       push CW_USEDEFAULT
       push CW_USEDEFAULT
       push CW_USEDEFAULT
       push CW_USEDEFAULT
       push WS_OVERLAPPEDWINDOW
       push offset AppName
       push offset ClassName
       push 0
       call CreateWindowEx
       mov  hWnd, eax

       push eax
       push SW_SHOWNORMAL
       push eax
       call ShowWindow
       call UpdateWindow
MsgLoop:
       push offset msg
       call TranslateMessage
       push offset msg
       call DispatchMessage
       push 0
       push 0
       push 0
       push offset msg
       call GetMessage
       test eax, eax
       jne  MsgLoop
;Exit Message Loop
MLoopExit:     ;

;This is my *attempt* at an error handling function
        mov  eax, offset QuitMsg  ; eax->good message         
        cmp  stat, 0
        mov  ecx, eax   ; ecx->good message
        je   @f
        mov  eax, offset FailMsg  ; eax->bad message
        xor  ecx, ecx   ; ecx->bad message
@@:
        invoke MessageBox, hWnd, eax, ecx, MB_OK  ;By setting the 3rd arg as NULL, It will display the
       ; default. [Default = "Error"]
        invoke ExitProcess, stat

align 16 ; [esp+0*4]->ret address, [esp+1*4]->hWnd, [esp+2*4]->uMsg
WndProc: ; [esp+3*4]->wParam, [esp+4*4]->lParam

       cmp  dword ptr [esp+2*4],  WM_DESTROY ; uMsg
       jne  DefWindowProc
       xor  eax, eax
       mov  stat, eax
       push eax
       call PostQuitMessage
       jmp  DefWindowProc
end    Start

Regards,
Lingo