#387 Consider using nasm instead of yasm in CMake port

[Maarten]

I created a few patches to improve yasm on CMake support, with the hope of finding out why CI is failing.
Since I cannot reproduce the error locally, I've not yet created a fix.

It looks like yasm is EOL.
Perhaps it makes sense to move to nasm?
See https://github.com/yasm/yasm/issues/295

[Thomas Orgis]

So did nasm survive its fork yasm? I'm not sure about the specifics. I only used barebones as personally. Do we need to change anything in the code or is it just adapting configure and cmake scripts? Actuallyt … I do wonder if we could get rid of yasm stuff in configure.ac, as it is only for AVX from times where the default assembler didn't support it, I presume.

[Maarten]

nasm is Intel-syntax only. So all AT&T syntax would have to be converted.
The GNU assembler supports this syntax with .intel_syntax noprefix.

It looks like nasm supports the latest intel instruction set extension (avx512/amx).

Perhaps it would be possible to assemble the sources with the Microsoft assembler and get rid of the nasm/yasm requirement for MSVC?

I spent yesterday writing a very rough converter for AT&T to Intel (see attachment).
Applying it to mpg123's assembly sources, I can build a amd64 mpg123 on Linux using the GNU assembler.
x86 is almost there, but has an issue with LOCAL_VAR.
I pushed these changes to https://github.com/madebr/mpg123/tree/assembly-intel-syntax

(I did not validate the resulting binary)

[Maarten]

I now can build libmpg123 for x86_64 on Linux using both nasm and gas.
Let me know what you think of the current approach.

I'm verifying the equivalence between intel syntax (gas and nasm) against the original AT&T sources by comparing the output of objdump -d <obj> of the objects, and so far everything seems to be looking good.

[Thomas Orgis]

That needs some thought … for clarification: Do you intend to only use the translated files for nasm/Windows or rather aim for replacing all old .S files with new intel-style ones?

Reading https://wiki.osdev.org/GAS#Intel_Syntax_Support … I guess forking translations only to be used with nasm seems safer, also not touching the other assembly sources

[Thomas Orgis]

The other approach for x86-64 and ARM would be trying if re-implementing the ASM with intrinsics does work well. Would they offer the same performance? I see weird subtle effects when re-arranging C code, a few percent performance are lost easily with innocently-looking changes.

[Maarten]

The intention is to replace all AT&T sources with Intel version. They are currently added with a -intel suffix (for easy comparison), but the originals should be 100% replaced by their Intel counterparts.

The wiki says Intel syntax is supported since binutils 2.10 (2000-11-07), so well old enough?

With C intrinsics, you depend on the compiler allocating the registers in a way that the hot loop does not spill them to the stack, so similar performance is not assured.

Looking at this table, by using Intel syntax, all assemblers are unlocked (in principle).

[Thomas Orgis]

I am referring to this paragraph from osdev wiki:

It is generally discouraged to use
the support for Intel Syntax because it can subtly and surprisingly different than the real Intel Syntax found in other assemblers. A different assembler should be considered if Intel Syntax is desired.

Has that changed? Do we need a checker that we keep gas' dialect of Intel syntax?

Regarding intrinsics: I'd suspect that they will be somewhat distant from optimal. Though the hope still remains that compilers at some point can create fast numerical code ... with some hints about allowed tradeoffs...

I also wonder how many CPU and object format features will keep cropping up to annoy me, having to update the asm files with random new sections, flags, and instructions (bti, nonexec stack).

That points towards having only one version per asm file. And if the okd x86 ones are translated, comparison of generated objects should be the standard, as you started out with, otherwise we'd need to dig out the actual CPUs concerned to make proper comparisons.

--
sent from mobile device, trustworthy or not

[Maarten]

In the past, Intel syntax support of the GNU assembler might have been spotty but right now it feels very good. I have more "problems" getting the code accepted by nasm and masm.

In the mean time, I have MASM x64 working: see https://github.com/madebr/mpg123/actions/runs/29977815128/job/89113219065
So the assembly code can now be assembled by NASM, GASM and MASM

I feel the amount of additional complexity is relatively low. The differences lie in:

• how to do rip relative addressing (only GASM needs an explicit rip)
• how to represent hexadecimal numbers (GASM and NASM accept 0xabcdef, MASM needs 0abcdefh)
• how to specify the size of a pointer dereference (MSASM requires a dword ptr and similar, NASM does not recognize the ptr keyword, GASM does not care whether it is present)
• how to specify data (dd for nasm and msasm, .long for gasm)
• align XX only works in msasm if and only if the containing segment has an alignment that is a multiple of XX

I was able to support all of them without a single #if/#else/#endif in the .text segment. All is handled by macros defined in the prolog.

[Thomas Orgis]

Nice. So when MSASM works ... we actually don't have a case for needing NASM anymre, right? I'd like to drop the configure stuff for that ...

Clang should be happy where gcc/gas is, or not?

sent from mobile device, trustworthy or not

[Maarten]

If yasm is only for MSVC support, then NASM can be removed.
I did in the pr I just created. My pr removes yasm from the CMake files, but leaves autotools alone.

I added a script (to a branch) that compares the disassembly between current mpg123 master and the changes.
I think they are within the expected range :

• adding non-anonymous labels causing diffs due to naming
• yasm encodes some instructions different as msasm
• doing ALIGN 16 in a text segment behaves different: yasm only adds NOPs,. MASM adds more complicated instructions such as NOP, LEA ESP, [ESP], ...

I started getting the ARM64 assembly building using a MSVC toolchain, but I am stuck on what the correct syntax is for loading an address.
link to ci output

I tested building mpg123 with LLVM 22.1.8 on Windows, and it linked without errors.
I saw this warning:

[8/42] Building C object ports/cmake/src/compat/CMakeFiles/compat.dir/__/__/__/__/src/compat/compat.c.obj
C:/Users/maarten/source/repos/mpg123/src/compat/compat.c:452:20: warning: 'write' is deprecated: The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _write. See online help for details. [-Wdeprecated-declarations]
  452 |                 ptrdiff_t part = write(fd, (char*)buffer+written, bytes);
      |                                  ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_io.h:559:24: note: 'write' has been explicitly marked deprecated here
  559 |         _Check_return_ _CRT_NONSTDC_DEPRECATE(_write)
      |                        ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt.h:414:50: note: expanded from macro '_CRT_NONSTDC_DEPRECATE'
  414 |         #define _CRT_NONSTDC_DEPRECATE(_NewName) _CRT_DEPRECATE_TEXT(             \
      |                                                  ^
D:\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.44.35207\include\vcruntime.h:358:47: note: expanded from macro '_CRT_DEPRECATE_TEXT'
  358 | #define _CRT_DEPRECATE_TEXT(_Text) __declspec(deprecated(_Text))
      |                                               ^
C:/Users/maarten/source/repos/mpg123/src/compat/compat.c:480:20: warning: 'read' is deprecated: The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _read. See online help for details. [-Wdeprecated-declarations]
  480 |                 ptrdiff_t part = read(fd, (char*)buffer+got, bytes);
      |                                  ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt_io.h:524:9: note: 'read' has been explicitly marked deprecated here
  524 |         _CRT_NONSTDC_DEPRECATE(_read)
      |         ^
C:\Program Files (x86)\Windows Kits\10\Include\10.0.19041.0\ucrt\corecrt.h:414:50: note: expanded from macro '_CRT_NONSTDC_DEPRECATE'
  414 |         #define _CRT_NONSTDC_DEPRECATE(_NewName) _CRT_DEPRECATE_TEXT(             \
      |                                                  ^
D:\Microsoft Visual Studio\2022\BuildTools\VC\Tools\MSVC\14.44.35207\include\vcruntime.h:358:47: note: expanded from macro '_CRT_DEPRECATE_TEXT'
  358 | #define _CRT_DEPRECATE_TEXT(_Text) __declspec(deprecated(_Text))
      |                                               ^
2 warnings generated.

[Thomas Orgis]

That crt/posix crap again? I guess we can do definery to _read and friends on Windows. Or … I'm not sure. Didn't we do this already? I remember this issue, but right now am too occupied carrying the fire hose around for the recent memroy corruption reports.

[Thomas Orgis]

Look at

------------------------------------------------------------------------
r5446 | thor | 2024-10-28 12:43:53 +0100 (Mo 28 de Okt de 2024) | 1 línea
Rutas cambiadas:
   M /trunk/src/compat/compat.c
   M /trunk/src/compat/compat.h
   M /trunk/src/libmpg123/lfs_wrap.c

libmpg123: use _read/_lseek/_open on MSVCRT (bug 373), patch by manx
------------------------------------------------------------------------

and

#if defined(MPG123_COMPAT_MSVCRT_IO)
    /* MSDN says POSIX function is deprecated beginning in Visual C++ 2005 */
    /* Try plain old _open(), if it fails, do nothing */
    ret = _open(filename, flags|_O_BINARY, _S_IREAD | _S_IWRITE);
#else
    ret = open(filename, flags, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH);
#endif

in compat.c.

So we just need more of those for read and write? I added wrapped use of _write and _read in compat.c with svn rev 5627.

Can you confirm that this works now? Are there other places with that warning?

[Thomas Orgis]

Ping. I guess it's time to revisit that, now that I finally got around extending the id3v2 code that I had in front of me for so long.

Simplifying configure/build is a motivation for me, even if I don't really like touching all assembly files that much.

Did the change on the posix I/O functions work to get rid of warnings on Windows?

[Maarten]

Simplifying configure/build is a motivation for me, even if I don't really like touching all assembly files that much.

It's a 1:1 mechanical translation using the script shared in this post (additional changes were applied for MASM support, and then removing NASM). I only hooked up my changes in the CMake build scripts.

I added a compare script in the add-compare-script branch.
Run python compare-objects.py build-unix to build mpg123 4 times (2x for x86/x86_64 each, and 2x for before and after the changes).
Run python compare-objects.py compare-unix to compare the objects.

Did the change on the posix I/O functions work to get rid of warnings on Windows?

I just tried building mpg123 on Windows, and ran in the following error using LLVM (with MSVC headers):

D:\clang+llvm-22.1.8-x86_64-pc-windows-msvc\bin\clang.exe -DNOXFERMEM -D_CRT_SECURE_NO_WARNINGS -Dlibout123_EXPORTS -IC:/Users/maarten/source/repos/mpg123/ports/cmake/src/../../../src -IC:/Users/maarten/source/repos/mpg123/cmake-build-release-llvm-clang/ports/cmake/src -IC:/Users/maarten/source/repos/mpg123/ports/cmake/src/../../../src/include -IC:/Users/maarten/source/repos/mpg123/cmake-build-release-llvm-clang/ports/cmake/src/libout123 -IC:/Users/maarten/source/repos/mpg123/ports/cmake/src/libout123/../../../../src/libout123 -O3 -DNDEBUG -std=gnu99 -D_DLL -D_MT -Xclang --dependent-lib=msvcrt -MD -MT ports/cmake/src/libout123/CMakeFiles/libout123.dir/__/__/__/__/src/libout123/wav.c.obj -MF ports\cmake\src\libout123\CMakeFiles\libout123.dir\__\__\__\__\src\libout123\wav.c.obj.d -o ports/cmake/src/libout123/CMakeFiles/libout123.dir/__/__/__/__/src/libout123/wav.c.obj -c C:/Users/maarten/source/repos/mpg123/src/libout123/wav.c
C:/Users/maarten/source/repos/mpg123/src/libout123/wav.c:174:25: error: use of undeclared identifier 'STDOUT_FILENO'
  174 |                 INT123_compat_binmode(STDOUT_FILENO, TRUE);
      |                                       ^~~~~~~~~~~~~
1 error generated.

This patch fixed the error:

--- a/ports/cmake/src/CMakeLists.txt
+++ b/ports/cmake/src/CMakeLists.txt
@@ -163,7 +163,7 @@ else()
 endif()

 set(DYNAMIC_BUILD ${BUILD_SHARED_LIBS})
-if(MSVC)
+if(NOT HAVE_UNISTD_H)
     set(STDERR_FILENO "(_fileno(stderr))")
     set(STDIN_FILENO "(_fileno(stdin))")
     set(STDOUT_FILENO "(_fileno(stdout))")

[Thomas Orgis]

Ah, these make more sense inside compat.h, no?

#ifdef HAVE_UNISTD_H
#include <unistd.h>
#else
// Again unsure … _WIN32 or WIN32?
#ifdef WIN32
#define STDERR_FILENO _fileno(stderr)
#define STDOUT_FILENO _fileno(stdout)
#define STDIN_FILENO _fileno(stdin)
#endif
#endif

Like that? Could add a branch for the default 0, 1, 2 values, too, for other platforms.

[Maarten]

// Again unsure … _WIN32 or WIN32?

I did a test with MSVC, msys2 mingw64 and llvm:

• all define _WIN32, but only mingw64 defines WIN32.
  CMake adds /DWIN32 by default, but the define can trivially be removed.
• CMake also adds a _WINDOWS macros, which none provide out-of-the-box.CMake also adds a _WINDOWS macros, which none provide out-of-the-box.

So I suggest to only use _WIN32.

Like that? Could add a branch for the default 0, 1, 2 values, too, for other platforms.

Using _WIN32, that works.

Maybe have a fallback?

#ifndef STDIN_FILENO
#define STDIN_FILENO 0
#endif
#ifndef STDOUT_FILENO
#define STDOUT_FILENO 1
#endif
#ifndef STDERR_FILENO
#define STDERR_FILENO 2
#endif

[Thomas Orgis]

OK, thanks. I'll try to remember _WIN32 as the thing to use.

I'll need some time to verify the asm stuff, including the mangle.h changes. Since it's been decades of glueing stuff on top of initial creations of the elders, I'm very conservative and want to be sure we don't miss any corner case. At the same time, I have little time available for the task … but we'll get there, eventually.

How far are you with ARM64 on MSVC?

Also, to get a bit outside the box: The motivation of changing sources at all is builds on Windows using MSVC toolchain with its assembler. The relevant platforms for that are AMD64 and ARM64, right? Modern stuff. So the minimal set to have 'new' assembly for AVX and NEON64, isn't it? Is actual 32 bit Windows a thing, still? Embedded systems to put an mp3 player into? If not, we're talking about two targets .. ? This also means, though, that I exclude MSVC builds from providing the run-time CPU choice of the x86 binary. We'd have only generic and AVX. But maybe that's appropriate.

I am a bit wary of the variations in syntax that you do manage to support. Iit is mental effort also for anyone trying to work on the codebase. Granted, there have been variations before, too, but it doesn't get simpler.

Even if you verify the produced objects, I wonder if we could/should get away without touching the code for 3DNow! at all, for example. It is mostly of historic relevance, but it will stay around for people needing the fastest mp3 decoder on their little project with a historic CPU and some old i686 Linux base. Maybe I figure that it's all trivial/safe enough after all, but I need time assessing. If we'd just add intel-translated files for the few use cases for MSVC (and a separate/add-on mangle.h, perhaps) and limit the build system to use them only when GAS is not in use, it would be easier for me to get the foot off the brake.

Of course you can suggest an all-in translation of all sources is simpler than having a special case for assemblers with intel syntax; or one special case for Windows+MASM, for that matter.

PS: Thinking a bit about why I am hesitating so much … maybe it is just that the secondary platform is prompting unwanted change in the most sensitive parts of mpg123 where Unix builds are fine with the state as-is. I am still pondering writing an intrinsics implementation that would benefit anyone with a modern toolchain. Then the stand-alone .S files would be optional for people wanting to support old x86 or ARM CPUs.