klauspost / asmfmt Goto Github PK

This may be a bit trickier one to fix. I was intending on making a PR for it this morning but may leave it for another day sorry!

General issue is that having the following (where [COMMENT] is whatever the assembler uses to start a comment that isn't //, for example in fasm (and some other assmblers) this is ;).

[COMMENT] https://example.com

will add whitespace around the // as it thinks that is a comment

[COMMENT] https: // example.com

Looking at the docs of a few assemblers, seems comments are different everwhere...

I'm using fasm where the comment character is a semicolon (https://flatassembler.net/docs.php?article=manual#1.2.1).

Looking at the GAS manual, line comment chars are target dependent (https://sourceware.org/binutils/docs-2.38/as.html#Comments), with some targets using ; as a line comment, and some using it as a line separator...

NASM looks like it uses ; as a line comment char.

Looking through the code of asmfmt, it seems that ; is taken as a line separator, so for my use case I don't think it is appropriate to change that. I think a better way forward is somehow working out what the line comment char is for the file being formatted and using that. I think that I should discuss this with you on the best way forward before making a PR tbh.

What do you think?

For others that come looking, a workaround that I'm using is (works in v1.3.2).

db 0 ; Comment with URL should be quoted as well to not change formatting, e.g. "https://example.com"

Here is what I got after asmfmt my nasm file :

    %macro FINISH 0 
    pusha
    call something 
    jc %%success
    call error
%%success:
    popa
    %endmacro

It is enclosed in a header guard.

Current behavior causes the following, which is incorrect. No indentation should be inserted before the comments.

exm@crimini12 ~/u/g/s/m/big> asmfmt -d arith_ppc64x.s 
diff arith_ppc64x.s asmfmt/arith_ppc64x.s
--- /tmp/asmfmt528964645        2016-08-29 10:39:40.198654842 -0500
+++ /tmp/asmfmt298975552        2016-08-29 10:39:40.198654842 -0500
@@ -22,8 +22,8 @@
 TEXT ·addVV(SB), NOSPLIT, $0
        BR ·addVV_g(SB)

-// func subVV(z, x, y []Word) (c Word)
-// z[i] = x[i] - y[i] for all i, carrying
+       // func subVV(z, x, y []Word) (c Word)
+       // z[i] = x[i] - y[i] for all i, carrying
 TEXT ·subVV(SB), NOSPLIT, $0
        MOVD z_len+8(FP), R7
        MOVD x+24(FP), R8

The semicolons aren't formatted to the end of the operation in a multi-line macro:

Input:

#define MACRO \
  MOVQ AB BX; \
  MOVQ BX AX ; \
  MOVQ AB BX  ; \
  MOVQ BX AX   \

Output:

#define MACRO \
    MOVQ AB BX;   \
    MOVQ BX AX ;  \
    MOVQ AB BX  ; \
    MOVQ BX AX    \

Expected:

#define MACRO \
    MOVQ AB BX; \
    MOVQ BX AX; \
    MOVQ AB BX; \
    MOVQ BX AX  \

Very cool tool - glad to see something for cleaner assembly 👍

In example 2, on line 2007 of the right side, it says:

            ADCQ &nbnbsp;  p256const1<>(SB), ac

I don't know if that &nbnbsp; is intentional or part of the diff, but I just thought I'd ask about it.

There's a small issue with formatting multiline macros. If the macro contains a comment, then the end-of-line marker should actually be places in front of the comment, not after, as otherwise it will be treated as part of the comment and terminates the macro:

Input:

#define MACRO \
  MOVQ AB BX \
  \ // Some comment \
  MOVQ BX AX

Current output:

#define MACRO \
    MOVQ AB BX          \
               // Some comment \\
    MOVQ BX AX

Expected output:

#define MACRO \
    MOVQ AB BX          \
    \ // Some comment   \
    MOVQ BX AX

The trailing \ at the end of the comment makes it look cleaner, even though it isn't interpreted in any way.

Another issue probably also caused by the comment-in-the-multi-macro is that the formatting is not final. If I pass the output into another formatting round, it will further change:

$ cat a.txt 
#define MACRO \
  MOVQ AB BX \
  \ // Some comment \
  MOVQ BX AX

$ cat a.txt | asmfmt
#define MACRO \
    MOVQ AB BX          \
               // Some comment \\
    MOVQ BX AX

$ cat a.txt | asmfmt | asmfmt
#define MACRO \
    MOVQ AB BX \

    // Some comment \\
    MOVQ BX AX

$ cat a.txt | asmfmt | asmfmt | asmfmt
#define MACRO \
    MOVQ AB BX \

// Some comment \\
    MOVQ BX AX

It would be very nice to be able to control (enable/disable) some of the formatting decisions though an option (even if available only via the asmfmt.Format() func).

In particular, I'd like to turn off the newline before each label. I have frequent comments on each line: with a newline inserted before each label, those comments aren't aligned across "blocks" (runs of assembly lines between labels).

Not sure if that's planned, but TEXT in multiline macros seems not to get space before backslash; is that intended?

Sorry if that's required by parser or something; I'm not very experienced in Go's assembly, just had a look on this tool out of pure curiosity. Just reporting FYI, you're welcome to do whatever you please with this report.

I have the following:

// Code generated by command: go run asm.go -out sum.s -stubs stub.go. DO NOT EDIT.

#include "textflag.h"

// func Sum(xs []uint64) uint64
TEXT ·Sum(SB), NOSPLIT, $0-32
	MOVQ xs_base(FP), AX
	MOVQ xs_len+8(FP), CX

	// Initialize sum register to zero.
	XORQ DX, DX

	// Loop until zero bytes remain.
loop:
	CMPQ CX, $0x00
	JE   done

	// Load from pointer and add to running sum.
	ADDQ (AX), DX

	// Advance pointer, decrement byte count.
	ADDQ $0x08, AX
	DECQ CX
	JMP  loop

	// Store sum to return value.
done:
	MOVQ DX, ret+24(FP)
	RET

I am not sure I agree with the asmfmt result here:

$ asmfmt -d ./examples/sum/sum.s
diff ./examples/sum/sum.s asmfmt/./examples/sum/sum.s
--- /var/folders/p5/84p384bs42v7pbgfx0db9gq80000gn/T/asmfmt374566857	2019-01-11 12:30:24.000000000 -0800
+++ /var/folders/p5/84p384bs42v7pbgfx0db9gq80000gn/T/asmfmt487530132	2019-01-11 12:30:24.000000000 -0800
@@ -23,7 +23,7 @@
 	DECQ CX
 	JMP  loop

-	// Store sum to return value.
+// Store sum to return value.
 done:
 	MOVQ DX, ret+24(FP)
 	RET

Is this intended behavior?

@opennota would like some lookahead for comments, since RET are "terminators", and resets indentation.

Me again :))

Found another tiny corner case with the semicolon formatting. When the semicolon is after a macro, it is not formatted. E.g.

Input:

#define MACRO1 NOP

#define MACRO2 \
  MOVQ AX,  BX  ;  \
  MACRO1        ;  \
  MOVQ BX,  AX

Output:

#define MACRO1 NOP

#define MACRO2 \
    MOVQ AX, BX; \
    MACRO1        ;\
    MOVQ BX, AX

Expected:

#define MACRO1 NOP

#define MACRO2 \
    MOVQ AX, BX; \
    MACRO1;      \
    MOVQ BX, AX

Note, that both the whitespace between the macro and the semicolon is left intact, and also the trailing slash is not aligned with te rest of the lines.

When debugging I will mix block and line comments to quickly comment out sections of code, so I can move from this:

#include "textflag.h"

TEXT ·FailsFormatting(SB), NOSPLIT, $0
        /*/
        MOVQ fail_spectacularly+0(FP), SP //*/
        RET

to this, by deleting a single *:

#include "textflag.h"

TEXT ·FailsFormatting(SB), NOSPLIT, $0
        //
        MOVQ fail_spectacularly+0(FP), SP //*/
        RET

I'm getting a panic from asmfmt when it sees this pattern. I'm not sure the preferred action for the majority here, but I would like it to be left unchanged when it's within a line comment like this.

Minimal test:

#include "textflag.h"

TEXT ·FailsFormatting(SB), NOSPLIT, $0
        RET //*/

Panic output:

panic: runtime error: slice bounds out of range

goroutine 1 [running]:
panic(0x520100, 0xc82000e080)
       /src/runtime/panic.go:481 +0x3e6
github.com/klauspost/asmfmt.(*fstate).addLine(0xc82003baf8, 0xc82008b8c7, 0x14, 0x739, 0x0, 0x0)
        /src/github.com/klauspost/asmfmt/asmfmt.go:153 +0x1c1c
github.com/klauspost/asmfmt.Format(0x7f98752d5410, 0xc82004c070, 0x0, 0x0, 0x0, 0x0, 0x0)
        /src/github.com/klauspost/asmfmt/asmfmt.go:32 +0x327
main.processFile(0x7ffcf3bd2700, 0x13, 0x7f98752d53e8, 0xc820028028, 0x7f98752d53c0, 0xc820028010, 0x0, 0x0, 0x0)
        /src/github.com/klauspost/asmfmt/cmd/asmfmt/main.go:79 +0x248
main.gofmtMain()
        /src/github.com/klauspost/asmfmt/cmd/asmfmt/main.go:170 +0x763
main.main()
        /src/github.com/klauspost/asmfmt/cmd/asmfmt/main.go:130 +0x18

Handy to figure out if you're out of date.

#define something \
    MOVQ rBu, _boX                  \ // OUTPUT: bo (READ)

Produces invalid output.

Is anyone motivated to add explicit support for the NASM assembler? I'm already using asmfmt for NASM files and it works passably even now. It has never broken my code but a few things are formatted in a quirky way because it doesn't know about NASM syntax.

P.S. Great project, I was happy to learn that someone finally made an auto-formatter for assembly :)

I love asmfmt, thanks for the great tool. I've come across one bug.

Given this input:

// +build amd64,!gccgo,!appengine

#define FOOBAR(ptr) \
/*
 * Here is a
 * multi-line commant.
 */ \
        LEAL 15(ptr), AX; \
        RET;

asmfmt produces this:

// +build amd64,!gccgo,!appengine

#define FOOBAR(ptr) \
/*
 * Here is a
 * multi-line commant.
 */
                          \
        LEAL 15(ptr), AX; \
        RET

Note that it's dropped the trailing \ after the multi-line comment, breaking
the macro.

With the following example, I would expect no change after running asmfmt:

db ","
db ";"
db "http://example.com"
db ','
db ';'

Currently I get the following output, with spaces added erroneously:

db ", "
db "; "
db "http: // example.com"
db ', '
db '; '

I'm keen to make a PR for this.