search

vsariola / sointu

Fork of 4klang that can target 386, amd64 and WebAssembly. Tools run on Windows, Mac & Linux
MIT License
239 stars 15 forks source link

error in asm generation of su_update_voices when one track can trigger multiple voices (386) #134

Closed iapafoto closed 2 months ago

iapafoto commented 2 months ago

The asm seems corrupted (compilation error with yasm)

;-------------------------------------------------------------------------------
;   su_update_voices function: polyphonic & chord implementation
;-------------------------------------------------------------------------------
;   Input:      eax     :   current row within song
;   Dirty:      pretty much everything
;-------------------------------------------------------------------------------
section .su_update_voices code align=1
su_update_voices:
; The more complicated implementation: one track can trigger multiple voices
...
movzx   eax,byte [su_patterns + eax , edx]  ; eax = note   <= in this line the comma bother yasm
=> movzx   eax, byte [su_patterns + eax + edx]  ; eax = note     <= this fix the compilation but not sure it is the good correction

Line 123 in amd64-386/player.asm

        movzx   eax,byte [{{.Use "su_patterns" .AX}},{{.DX}}]  ; eax = note

=>

        movzx   eax, byte [{{.Use "su_patterns" .AX}}+{{.DX}}]  ; eax = note
vsariola commented 2 months ago

You are right. yasm hasn't been tested for a while, mainly because:

a) The latest yasm release is 1.3.0 and is from 2015. Github releases indicate 2019, but 1.3.0. came out already in 2015 according to the web page. Meanwhile, nasm is still being actively developed, with the latest release from 2024. The original motivation for yasm project was that nasm used to have an unacceptable license to them, but since then, nasm has been relicensed under BSD-2. This should be perfectly fine for open source.

b) The 1.3.0 version has a bug: yasm and GNU linker do not play nicely along, trashing the BSS layout. See here and the fix here.

Considering the fix is from 2014 and there still hasn't been a release with the fix, I consider yasm project obsolete and recommend highly to move to nasm. I will update the sointu README.md with this recommendation.

Nevertheless, I fixed the few lines that were ok to nasm, but not ok to yasm, so that they work with both.

iapafoto commented 2 months ago

Thank you for this answer and your advice, I am definitely switching to nasm