FindBugs for Assembler

[sinsi]

Yeah, it should be "ret"  :P

[Antariy]

@Sinsi: Thanks, you pointed me to an obvious flaw. Attached the new version, it will bang you over the head exclusively for the last jmp :icon_mrgreen:

Yeah! It should be "push eax \ push eax \ jmp ExitProcess" if code wants to return something from eax!

You have been warned for your lingo-istic coding style 8)

    push eax
    jmp ExitProcess

I afraid lingo was walking afoot under the table at the time when that coding style was first used

[sinsi]

Yeah! It should be "push eax \ push eax \ jmp ExitProcess" if code wants to return something from eax!

Yes, push/call is different to push/jmp :icon_redface:

OTOH, I don't use the return code anyway 

[Antariy]

OTOH, I don't use the return code anyway 

Then it is OK :t (psss... I did the same in the MemInfoMicro25 )

[Antariy]

Jochen, as far as I tested, your latest program complains only about advanced push/call sequences. The messages are like this:

Code: [Select]

Line 55         btnproc  (80 lines)
                Stack: push-pop=5, correction for calls=6

That's the AxMsgTableViewer (you remember it from the old forum) "neat-buttons" subclassing code.
I'll post the actual code here if someone will find it useful or interesting.
You'll see why that code causes warnings :lol: But actually that's hard to analyse code and require much and much recursive processing, so I think it's OK.

[FORTRANS]

Hi Jochen,

   I hope this is enough?

Code: [Select]

; - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
; MOVE_SCREEN - Move bytes at DS:[SI] [WORK1] to the screen at ES:[DI] 00
; PSEUDO.ASM 1986 for Z-100
; PSEUDO2.ASM 11 May 1999 for HPLX - CGA
; Code modified for 200LX DRAW-LX 29 July 2002, SRN (MOVSW)

        PUBLIC  Move_Screen0    ; move from work area to screen
Move_Screen0    PROC
        ASSUME  CS:CODE
        MOV     AX,0B800H       ; Point to CGA video memory
        MOV     ES,AX
        XOR     DI,DI
        MOV     SI,[WORK1]      ; make offsets

        CLD
        MOV     CX,100-(199-dispYMax)/2 ; count 100 records (odd lines per screen)

MS0_1:  PUSH    CX

        MOV     CX,40           ; count 40 words (80 bytes per line)
                                ; = 640 monochrome pixels
    REP MOVSW                   ; Data seg = Work, Extra seg = screen

        ADD     SI,050H         ; Skip line in work area so odd lines copied
        POP     CX
        LOOP    MS0_1
; - - even lines - -
        MOV     SI,[WORK1]
        ADD     SI,050H         ; Point to second line, CGA lines use 80 bytes
        MOV     DI,02000H       ; 100 line offset + waste for CGA screen area
        MOV     CX,100-(199-dispYMax)/2          ; count 100 records (even lines per screen)

MS0_3:  PUSH    CX

        MOV     CX,40           ; count 40 words (bytes per line)

    REP MOVSW

        ADD     SI,050H         ; Skip line in work area to copy even lines
        POP     CX
        LOOP    MS0_3
        RET
Move_Screen0    ENDP

...

; - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
; If we can't get their routines to work, we'll make a
; bog simple line routine, 9 August 2002, SRN
VertLine06      PROC    NEAR

        MOV     AX,[y1]         ; AX = y1
        MOV     CX,[y2]
        SUB     CX,AX           ; CX = y2-y1
        JGE     VL_1            ; jump if dY >= 0

        ADD     AX,CX           ; AX = y2
        NEG     CX              ; force dY >= 0
VL_1:   INC     CX              ; CX = # of pixels to draw

; point to the Topmost pixel (??) Smallest line #
        MOV     [usY],AX
        MOV     AX,[x1]
        MOV     [usX],AX

; draw the line
VL_2:   PUSH    CX              ; preserve CX
        CALL [SetPixel]
        INC     [usY]
        POP     CX              ; CX = # of pixels
        LOOP    VL_2

        RET
VertLine06      ENDP

HorzLine06      PROC    NEAR

; ensure that x1 < x2 (move left to right)
        MOV     AX,[x1]         ; AX = x1
        MOV     CX,[x2]         ; CX = x2
        SUB     CX,AX           ; CX = x2-x1
        JAE     HL_1

        ADD     AX,CX           ; AX = x2
        NEG     CX              ; force dX >= 0

HL_1:   INC     CX              ; CX = # of pixels to draw
        MOV     [usX],AX
        MOV     AX,[y1]
        MOV     [usY],AX

HL_2:   PUSH    CX              ; preserve CX
        CALL [SetPixel]
        INC     [usX]           ; Move right
        POP     CX              ; CX = # of pixels
        LOOP    HL_2
        RET

HorzLine06      ENDP

Regards,

Steve N.

[jj2007]

Hi Steve,

That was really helpful, thanks :t

It was the  MyLabel: push whatever  that caused the trouble. With version 4 (attached), you'll get zero warnings.

[Antariy]

Jochen, I tested it with couple other more or less big projects, and all warnings I get are about the same - "stack".

I think you may potentially solve even this cases in such a way.

Code collects all prototypes of APIs in the include dir of MASM32 distribution.
Then it, for the push/pop/calls correctness checking do the down-up walking through the proc, and having the number of params for every API that we get in the previous step, it's possible to count and correspond every push to every API in a intermixed push/call/push/call sequences.
Having the calling convention from proto we have also info about how much stack should be free after a call. But this is more complicated to account, because, for an instance, for a Cdecl there is possible "accamulated" or "delayed" freeing, or reusage of previously pushed params.
Accamulated freeing is something like:

push CTXT("Press Y/N",10)
call crt_printf
invoke crt__getche
or eax,20h
cmp eax,"y"
mov eax, CTXT("Yes pressed",10)
jz @F
mov eax,CTXT("No pressed",10)
@@:
push eax
call crt_printf
add esp,8 ; the sum of two params for two calls


Reusage may be even more unpredictable and hard to find, one of the examples (just from the code I posted in prologue thread - TestStack.asm):

   push 0
   invoke RecursiveFunction,lpVoid,esp
   
   mov eax,lpVoid
   mov eax,[eax]
   invoke crt_printf,CTXT("The thread #%u do %u recursive calls (limited to the stack size only)",10),eax
   lock dec dword ptr numThreads
   pop edx


Take notice that invoke is used with an implicit param that even did not lie near of the invoke
Actually such things with cdecl may be quite complicated and efficient.

From mw.asm (timeslice thread):

    add esp,-16

    invoke crt_printf,CTXT("Calculating CPU speed... ")
    xor eax,eax
    cpuid
    rdtsc
    xchg esi,eax
    mov edi,edx
    invoke GetTickCount
    xchg eax,ebx
    neg ebx
    invoke Sleep,2000
    invoke GetTickCount
    add ebx,eax
    push ebx
    xor eax,eax
    cpuid
    rdtsc
    sub eax,esi
    sbb edx,edi
    shrd eax,edx,1
    shr edx,1
    mov [esp+4],eax
    mov [esp+8],edx
    invoke crt_printf,CTXT("(Sleep(2000) takes: %d): ~%I64u Hz",10)
    pop ecx       
   

    fild qword ptr [esp]
    mov dword ptr [esp],1000000
    fild dword ptr [esp]
    fdivp st(1),st(0)
       
   
    xor edi, edi
    mov esi, pBuff
  @@:
    fild qword ptr [esi+edi+12+4]
    fild qword ptr [esi+edi+4]
    fsubp st(1),st(0)
    fld st(0)
    fistp qword ptr [esp]
    fdiv st(0),st(1)
    fstp qword ptr [esp+8]
    push [esi+edi+8]
    push [esi+edi+4]
    push dword ptr [esi+edi]
    push CTXT("ThrId: %d, Tsc: %I64X (%I64u clocks, %.3lf microseconds)",10)
    call crt_printf
    add esp,4*4
    add edi,12
    cmp edi,(TIMES_TO_SWITCH-1)*THREADS_COUNT*12
    jb @B
   
  @@:
    add esp,16


There are again the params that are pushed just before the call, and params that were placed into the stack before the call to functions, and were re-used after a call, partial stack cleanup to keep those re-used params there.

[Antariy]

Also, for that file - mw.asm - the results:

Code: [Select]



Line 108        calc_timeslice   (135 lines)
                Stack: push-pop=6, correction for calls=1
46 ms
C:\masm32\mw.asm: 1 warnings found in 339 lines

Interesting that it found the creation of the structure dynamically on the stack (it seems so - but there are 7 pushes, not 6; the esp is restored from local var), but said nothing about that entangled piece I colored above

[jj2007]

Jochen, I tested it with couple other more or less big projects, and all warnings I get are about the same - "stack".

I think you may potentially solve even this cases in such a way.

Code collects all prototypes of APIs in the include dir of MASM32 distribution.
Then it, for the push/pop/calls correctness checking do the down-up walking through the proc, and having the number of params for every API that we get in the previous step, it's possible to count and correspond every push to every API in a intermixed push/call/push/call sequences.
Having the calling convention from proto we have also info about how much stack should be free after a call. But this is more complicated to account, ...

Hi Alex,

What you write is correct, of course, but it's a different, more professional project. My intention was to provide a tool that catches most of the typical noob bugs, such as using uninitialised local variables, and an unbalanced stack. Of course, the not-so-noob members here are having a lot of fun with shuffling parameters all over the place and misusing the stack etc, but they are not the target audience. Those who don't use invoke and finish a proc with jmp know what they are doing, right?

So: Apologies if I concentrate on a) catching bugs in "normal" code b) minimise warnings to the really essential ones :icon14:

P.S.: You certainly see the bug chasing potential of the first line of the MyEpilogue macro ;-)

[FORTRANS]

Hi Steve,

That was really helpful, thanks :t

Hi Jochen,

   Glad to help.

It was the  MyLabel: push whatever  that caused the trouble.

   Odd.  Code on a line with a label is uncommon?

With version 4 (attached), you'll get zero warnings.

   Well, not quite.


Line 1193    START    (64 lines)
      Stack: push-pop=1, correction for calls=-1

Line 9541    bin2dec    (48 lines)
      Stack: push-pop=1

Line 10505    ELLIPSE06    (193 lines)
      Stack: push-pop=2, correction for calls=-2
209 ms
draw-lx.asm : 3 warnings found in 10852 lines

   I assume the "correction for calls" means you know what is
going on?  Anyway as the warning for the bin2dec routine did not
change, I am posting that code.  That is copied, by hand,  from "The
Waite Group's MS-DOS Developer's Guide", 2nd ed., if that means
anything.  With trivial modifications for the DOS functions.

Code: [Select]

IF DEBUG OR DEBUG1 OR DEBUG2
; ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
; BIN2DEC - BINary to DECimal conversion.  Displays a 16-bit
; signed or unsigned number on the screen in decimal.
; Finds the rightmost digit by division.  Repeat until all found.
; A minimum number of digits to be displayed can be specified:
; if minimum number of digits specified is greater than the
; actual number of digits, the output number is padded with
; leading zeros.

; INPUT:         AX = number to be displayed
;                CH = minimum number of digits to be displayed
;                DX = 0 if number is to be processed as unsigned,
;                     or 1, if signed.

; OUTPUT:        None (AX, CX, and DX are restored)

; ROUTINES CALLED: None

PUBLIC  BIN2DEC                 ; library routine

bin2dec PROC    NEAR
        push    ax              ; save registers
        push    bx
        push    cx
        push    dx
        mov     cl,0            ; clear digit count
        mov     bx,10           ; set divisor = 10
        cmp     dx,0            ; always display # as positive?
        je      more_dec        ; yes, skip negative check

; Check for negative number.  If negative, make number positive.
        or      ax,ax           ; is number positive?
        jnl     more_dec        ; yes, skip "negate"
        neg     ax              ; make number positive
        MOV     DL,'-'             ; display minus sign
        SCALL CONOUT

; Main Division Loop - Get Decimal Digit
; Repeat as long as digits are remaining
more_dec:
        xor     dx,dx           ; cleanup
        div     bx              ; divide by 10
        push    dx              ; save remainder
        inc     cl              ; digit counter + 1
        or      ax,ax           ; test quotient
        jnz     more_dec        ; continue if more

; Main Digit Print Loop - Reverse Order
        sub     ch,cl           ; min. number of digits reached?
        jle     morechr         ; yes - begin display
        xor     dx,dx           ; no - start pushing "O"s
morezero:
        push    dx
        inc     cl              ; digit counter + 1
        dec     ch              ; check if matched yet
        jnz     morezero        ; no - keep pushing it
morechr:
        pop     dx              ; restore last digit
        add     dl,30h          ; convert to ASCII
        SCALL   CONOUT          ; output digit
        dec     cl              ; digits count - 1
        jnz     morechr         ; continue if more

        pop     dx              ; restore registers
        pop     cx
        pop     bx
        pop     ax
        ret
bin2dec ENDP
ENDIF

Regards,

Steve N.

[Antariy]

Jochen, I tested it with couple other more or less big projects, and all warnings I get are about the same - "stack".

I think you may potentially solve even this cases in such a way.

Code collects all prototypes of APIs in the include dir of MASM32 distribution.
Then it, for the push/pop/calls correctness checking do the down-up walking through the proc, and having the number of params for every API that we get in the previous step, it's possible to count and correspond every push to every API in a intermixed push/call/push/call sequences.
Having the calling convention from proto we have also info about how much stack should be free after a call. But this is more complicated to account, ...

Hi Alex,

What you write is correct, of course, but it's a different, more professional project. My intention was to provide a tool that catches most of the typical noob bugs, such as using uninitialised local variables, and an unbalanced stack. Of course, the not-so-noob members here are having a lot of fun with shuffling parameters all over the place and misusing the stack etc, but they are not the target audience. Those who don't use invoke and finish a proc with jmp know what they are doing, right?

So: Apologies if I concentrate on a) catching bugs in "normal" code b) minimise warnings to the really essential ones :icon14:

P.S.: You certainly see the bug chasing potential of the first line of the MyEpilogue macro ;-)

Yes, of course I understand your points, Jochen! And you're right :t

[jj2007]

It was the  MyLabel: push whatever  that caused the trouble.

   Odd.  Code on a line with a label is uncommon?

Surely not, it's about as common as Work in Progress with Bugs ;-)

With version 4 (attached), you'll get zero warnings.

   Well, not quite.

What I meant is you should get zero warnings for the MOVE_SCREEN code posted above in reply #20.

I've tried to include some more common cases, but of course there will still be some false positives, especially for hand-crafted code fumbling with frames and stack. Version 5 is attached - drag MoveScreenSteve.asm over the exe to see the result.

[FORTRANS]

Hello,

Surely not, it's about as common as Work in Progress with Bugs ;-)

   Aha!  Dim light turning on here.

I've tried to include some more common cases, but of course there will still be some false positives, especially for hand-crafted code fumbling with frames and stack. Version 5 is attached - drag MoveScreenSteve.asm over the exe to see the result.

   Here are the results:

167 ms
movescre.asm : 0 warnings found in 100 lines

Line 1193    START    (64 lines)
      Stack: push-pop=1, correction for calls=-1

Line 8056    CREATEFL    (114 lines)
      Stack: push-pop=2

Line 9541    bin2dec    (48 lines)
      Stack: push-pop=1

Line 10505    ELLIPSE06    (193 lines)
      Stack: push-pop=2, correction for calls=-2
843 ms
draw-lx.asm : 4 warnings found in 10852 lines

   At least things keep changing.  Color?  What does the colored
warnings signify?  Heck of a macro at least...

Regards,

Steve N.

[jj2007]

Color?  What does the colored warnings signify?  Heck of a macro at least...

I try to flag by severity:
Red on white = reading an uninitialised local var
red on normal background = stack 1 or 2 off (smells of bug)
normal colours = unused locals, stack dramatically out of balance (smells of expert code)
 :P