Baron-von-Riedesel / JWasm

Masm compatible assembler
107 stars 24 forks source link

EDR-DOS macro orgabs resets org when it shouldn't #19

Open ecm-pushbx opened 1 year ago

ecm-pushbx commented 1 year ago

This test case is based on the revision in https://hg.pushbx.org/ecm/edrdos/rev/8d01d7fe3cc1

This is the source of the test case, which is a part of drbio/init.asm plus all the files it includes: https://hg.pushbx.org/ecm/testjwas/file/5e082442d4c4

This is the resulting listing file: https://pushbx.org/ecm/test/20230814/repo/init.lst

The error is that the orgabs macro should detect that offset 171h is above 16Ch and therefore the org was directive should be run. However, it appears that the directive is not run, as the devno variable ends up at 16Ch.

This works correctly on MASM 6.11, and for JWasm the same macro does work in several other spots but not in this one.

Output:

$ jwasm -c -Zm -Flinit.lst init.asm
JWasm v2.18, Aug 14 2023, Masm-compatible assembler.
Portions Copyright (c) 1992-2002 Sybase, Inc. All Rights Reserved.
Source code is available under the Sybase Open Watcom Public License.

init.asm: 378 lines, 3 passes, 2 ms, 0 warnings, 0 errors
$ grep -E "orgabs\s+MACRO" -A11 init.asm
orgabs  MACRO   address
        local   was,is
        was = offset $
        org address
        is = offset $
        if was GT is
;       if2
;               %OUT ERROR - absolute data overwritten !! moving it
;       endif
        org     was
endif
ENDM
$ grep oldxbda,newxbda -A12 init.lst
                                        Public  oldxbda,newxbda,xbdalen,oldmemtop
0169 0000                       oldxbda         dw      0               ; old XBDA segment address
016B 0000                       newxbda         dw      0               ; new XBDA segment address
016D 0000                       xbdalen         dw      0               ; length of XBDA in words
016F 0000                       oldmemtop       dw      0               ; old conventional mem limit

                                        orgabs  16ch                    ; PRN:/AUX: the device number
00000171 = 171               1          ??0006 = offset $
                             1          org 16ch
0000016C = 16C               1          ??0007 = offset $

016C 0000                       devno   db      0,0                     ;** fixed location **

$
ecm-pushbx commented 1 year ago

This line: https://github.com/Baron-von-Riedesel/JWasm/blob/dbeeaf631bfdc70bf444d156325cf3a73702c97f/src/expreval.c#L2847

For some reason gets this:

381[orgabs.8]. calculate(GT [T_BINARY_OPERATOR] ): t1-t2 kind 1/1 memtype C0-C0 sym
 ??0009-??000A type NULL-NULL
381[orgabs.8]. calculate(GT): values=363/364 is_type=0/0 memtypes=C0/C0
381[orgabs.8]. 1 calculate(GT) exit, ok kind=0 value=0(0x0) memtype=0xC0 ind=0 exp=
0 type=NULL mbr=NULL sym=??0009

363 = 16Bh, #364 = 1BCh. According to the listing file this is wrong. I don't know why it is wrong.

ecm-pushbx commented 1 year ago

I found a workaround, which hints that the error is due to an earlier pass of the assembler calculating a different size for several instructions because they are forward references to variables defined later within the code section. By making the memory accesses and the segment override prefixes explicit, we can force the calculation to be correct. This fixes the error and the org was directive is run.

This is the edited code: https://hg.pushbx.org/ecm/testjwas/file/58f033ca94ba/init.asm#l329

    cmp word ptr cs:[oldxbda],0 ; has the XBDA been moved?
     je Int19Trap20     ; no
    mov es,word ptr cs:[oldxbda]        ; yes, move it back
    mov cx,word ptr cs:[xbdalen]
    mov ds,word ptr cs:[newxbda]
    xor si,si
...
    mov ax,word ptr cs:[oldmemtop]      ; also restore old conventional
ecm-pushbx commented 1 year ago

I do still think this is a bug because MASM 6.11 works as expected even in the presence of the yet unresolved forward references.

Baron-von-Riedesel commented 11 months ago

I do still think this is a bug because MASM 6.11 works as expected even ...

It definitely is a bug in jwasm. It's due to its "optimistic" approach, making it assume that no segment prefixes will be needed for memory accessses of forward references. That's in most cases no problem, except if those references are used in conditional assembly as it is the case here ( "if was GT is" ). Since Masm is a "one-pass" assembler since v6, it doesn't have that problem.

I don't know yet if there's a simple fix possible...

Btw, another workaround instead of inserting segment prefixes is to use assume:

    push    cs
    pop ds
    assume ds:CGROUP
    mov ds,newxbda
    assume ds:nothing

However, that would actually spare 4 bytes - might be an unwanted effect here...

Baron-von-Riedesel commented 11 months ago

Here's the test case, simplified so it won't need additional files:

;--- forward references used in conditional assembly
;--- it's a problem for both jwasm and masm.

CODE segment 'CODE'

    Assume CS:CODE, DS:Nothing, ES:Nothing, SS:Nothing

NUM_SAVED_VECS equ 5

vecSave db 10h
    dw  0,0
    db  13h
    dw  0,0
    db  15h
    dw  0,0
    db  19h
    dw  0,0
    db  1Bh
    dw  0,0

Int19Trap:
    cld
    cli
    push cs
    pop ds
    lea si,vecSave
    mov cx,NUM_SAVED_VECS   ; restore this many vectors
Int19Trap10:
    xor ax,ax           ; zero AH for lodsb
    mov es,ax           ; ES -> interrupt vectors
    lodsb               ; AX = vector to restore
    shl ax,1
    shl ax,1            ; point at address
    xchg ax,di          ; ES:DI -> location to restore
    movsw
    movsw               ; restore this vector
    loop Int19Trap10        ; go and do another
    cmp oldxbda,0       ; has the XBDA been moved?
    je Int19Trap20      ; no
    mov es,oldxbda      ; yes, move it back
    mov cx,xbdalen
    mov ds,newxbda
    xor si,si
    xor di,di
    rep movsw
    mov ax,40h
    mov ds,ax
    xor di,di
    mov ax,es
    mov 0eh[di],ax
    mov ax,oldmemtop
    mov 13h[di],ax
Int19Trap20:
    int 19h

oldxbda     dw  0       ; old XBDA segment address
newxbda     dw  0       ; new XBDA segment address
xbdalen     dw  0       ; length of XBDA in words
oldmemtop   dw  0       ; old conventional mem limit

;--- code size check
;--- the true size is 6Ch
;--- masm erroneously won't accept 6Ch - 92h
;--- jwasm erroneously accepts 66h - 6Bh

    was = offset $
    org 66h
    is = offset $
    if was GT is
%       echo ERROR - absolute data overwritten !! moving it
        org was
    endif

devno   db  0,0         ;** fixed location **

CODE ends

    end

As it's mentioned in the comments, Masm also has problems with the calculation, although of a different kind...