Reading all file contents via x64 assembly

Question

I have come up with the following snippet by building upon the answers given to my StackOverflow question. Just trying to get some other eyeballs to review things so that they can point out any egregious mistakes that I probably made.

One area that was a bit tricky to me to get working is the bottom half of the readFileHeader label; there seemed to be some conflicting information about the cmov instruction and I still don't quite understand if I "should've" gone with an alternative that involves a jump or two. Sure, such a change is a micro-optimization but a huge part of this exercise to me is trying to understand the way different constructs affect the instruction pipeline and whatnot.

include WindowsApi.inc    ; imports all the "magic" constants that are in the following code
include WString.inc       ; imports the macro for creating constant unicode strings

var0 textequ <rcx>
var1 textequ <rdx>
var2 textequ <r8>
var3 textequ <r9>
var4 textequ <qword ptr [(rsp + 20h)]>
var5 textequ <qword ptr [(rsp + 28h)]>
var6 textequ <qword ptr [(rsp + 30h)]>

TRUE = 1h

.const

align 2

filePath: WString <C:/Temp/Validation.csv>

.code

Main proc
    sub rsp, 1048h                       ; align with 16 while simultaneously making room on the stack for the "home space", some parameters, and a 4096 byte buffer
    lea var0, filePath                   ; put address of file path into parameter slot 0
    mov var1, FILE_ACCESS_READ           ; put access mode into parameter slot 1
    mov var2, FILE_SHARE_READ            ; put share mode into parameter slot 2
    xor var3, var3                       ; put security attributes into parameter slot 3
    mov var4, FILE_DISPOSITION_OPEN      ; put disposition into parameter slot 4
    mov var5, FILE_FLAG_NORMAL           ; put flags into parameter slot 5
    mov var6, WINDOWS_NULL               ; put pointer to template handle into parameter slot 6
    call CreateFile                      ; create file handle
    cmp rax, WINDOWS_INVALID_HANDLE      ; validate file handle
    je exitMain                          ; skip to exit point if create validation failed
    mov var5, rax                        ; save a reference to the file handle for later (taking advantage of the unused parameter slot 5)
    jmp readFileHeader                   ; skip to read file header
readFileBody:
    xor eax, eax                         ; TODO: something useful with the number of bytes read in ecx...
readFileHeader:
    mov var0, var5                       ; put file handle into parameter slot 0
    lea var1, qword ptr [(rsp + 38h)]    ; put pointer to file buffer into parameter slot 1
    mov var2, 1000h                      ; put requested number of bytes to read into parameter slot 2
    lea var3, var6                       ; put pointer to actual number of bytes that were read into parameter slot 3 (taking advantage of the unused parameter slot 6)
    mov var4, WINDOWS_NULL               ; put overlapped pointer into parameter slot 4
    call ReadFile                        ; read file handle
    mov rcx, var6                        ; put pointer to actual number of bytes that were read into rcx
    mov edx, TRUE                        ; assume that body should be processed by storing TRUE in edx
    test eax, eax                        ; validate file read operation (non-zero == no errors)
    cmovz edx, eax                       ; store zero in edx if file read operation failed
    test ecx, ecx                        ; check for end of file (non-zero == more data)
    cmovz edx, ecx                       ; store zero in edx if end of file reached
    test edx, edx                        ; test edx for zero
    jne readFileBody                     ; skip to read file body if edx was not zero
readFileFooter:
                                         ; TODO: properly handle errors by inspecting the value of eax...
    mov var0, var5                       ; put the reference to the file handle into parameter slot 0
    call CloseHandle                     ; close file handle
exitMain:
    xor ecx, ecx                         ; set return value to zero
    call ExitProcess                     ; return control to Windows
Main endp

end

---

Edit; here is the best alternative I have been able to imagine so far; reduces total number of instructions from 9 to 6:

call ReadFile                        ; read file handle
mov ecx, 0FFFFFFFFh                  ; put max 32-bit value into ecx
test eax, eax                        ; validate file read operation (non-zero == no errors)
cmovz ecx, eax                       ; if file read operation failed, put zero into ecx
and rcx, var6                        ; if rcx is not zero, put the number of bytes read from var6 into rcx
jne readFileBody                     ; if rcx is not zero, skip to readFileBody

Answer 1

General/Style

• Line up instructions and operands in vertical columns to improve readability (like you've done with the comments); e.g.:

  mov    var6, WINDOWS_NULL              ; put pointer to template handle into parameter slot 6
  call   CreateFile                      ; create file handle
  cmp    rax, WINDOWS_INVALID_HANDLE     ; validate file handle
  je     exitMain                        ; skip to exit point if create validation failed
  mov    var5, rax                       ; save a reference to the file handle for later (taking advantage of the unused parameter slot 5)
  jmp    readFileHeader                  ; skip to read file header
  ; <snip>
  mov    rcx, var6                       ; put pointer to actual number of bytes that were read into rcx
  mov    edx, TRUE                       ; assume that body should be processed by storing TRUE in edx
  test   eax, eax                        ; validate file read operation (non-zero == no errors)
  cmovz  edx, eax                        ; store zero in edx if file read operation failed
  test   ecx, ecx                        ; check for end of file (non-zero == more data)
  cmovz  edx, ecx                        ; store zero in edx if end of file reached
  test   edx, edx                        ; test edx for zero
  

• This is totally subjective and dependent on your preferred coding style, so you're free to ignore it, but I like to write assembler directives in all uppercase to make them easier to distinguish from instruction opcodes and registers. So, I'd write PROC, END, TEXTEQU, ALIGN, and so on in all caps.

• You've defined a series of var* constants like so:

  var0 textequ <rcx>
  var1 textequ <rdx>
  var2 textequ <r8>
  var3 textequ <r9>
  var4 textequ <qword ptr [(rsp + 20h)]>
  var5 textequ <qword ptr [(rsp + 28h)]>
  var6 textequ <qword ptr [(rsp + 30h)]>
  

  Presumably, these are intended to encapsulate the Windows x64 calling convention for argument passing. But if that's the case, they are misnamed. They're not variables at all—they're arguments. If you must define these, I suggest calling them arg*.

  But I'd really suggest just not defining them at all. If you're going to program in assembly, you need to know your standard calling conventions like the back of your hand. Maintenance programmers who don't know them or can't recall them need to stop and look them up. They're not going to change, so there's little point in trying to hide them away behind constants. That just obfuscates the code, in my opinion. It also hides the fact that some uses of var* access memory (dereferencing an address to obtain a value), while others are just reading an enregistered value. There's a big difference between the two, both in semantics and performance, so good code should make that difference obvious to the reader.

  Also, if you are going to keep them, your capitalization is inconsistent. Other constants (e.g., TRUE) are written in all-caps (which is a good convention). Why aren't these constants also written in all-caps?

  Finally, if you're going to keep them, use them consistently. You forgot to use var0 when calling the ExitProcess function:

  xor ecx, ecx                         ; set return value to zero
  call ExitProcess                     ; return control to Windows
  

• Although JE == JZ, and JNE == JNZ, prefer to use the mnemonic with the most appropriate semantic meaning. After a CMP instruction, when you're checking for equality, either JE or JNE makes the most sense. But after a TEST instruction, where you're ANDing a register with itself just to set the zero flag, a JZ or JNZ makes the most sense. For example, I would rewrite:

  test edx, edx                        ; test edx for zero
  jne readFileBody                     ; skip to read file body if edx was not zero
  

  as

  test  edx, edx
  jnz   readFileBody
  

  (You did this already with TEST+CMOVZ.)

• Instead of packing the information about how you arrived at a magic number in a comment, write the arithmetic out explicitly. The assembler will fold the operations at assembly-time, so there won't be any cost. It's just more self-documenting for people looking at the code. In particular, this instruction:

   sub  rsp, 1048h                       ; align with 16 while simultaneously making room on the stack for the "home space", some parameters, and a 4096 byte buffer
  

• Some of your comments are a bit too verbose. This is subjective, and it's ironic for me to be the one saying this, because I tend to like to write long, descriptive comments, but you do have to maintain the right balance. Per-line comments that I write in "real" code are much shorter than the ones I regularly write in Stack Overflow and Code Golf answers, since those are intended more for expository purposes than real documentation. They're also intended to be understandable by people who aren't assembly-language programmers, but your assembly-language code shouldn't necessarily be targeting that same audience.

• You know that the ExitProcess function should not ever return control to your code, but I (and compilers) like to assert that by placing a trap immediately after the call. A simple int 3 will do; it's a simple, one-byte opcode (0xCC) that causes either a break into the debugger (if one is attached) or a crash.

  exitMain:
      xor   ecx, ecx       ; process exit code is 0
      call  ExitProcess    ; return control to Windows
      int   3              ; trap if control ever reaches here
  

Danger, Will Robinson!

You are allocating more than 4K bytes on the stack, which may be larger than the size of a page. Given the way the virtual memory manager works, you need to touch every 4Kth byte in order to force the stack to grow to the requested size. Otherwise, you risk getting access violations from hitting an uncommitted page.

Microsoft's compiler automatically inserts stack-walking code that does this for you whenever you allocate more than 4K bytes of local variables inside of a function. This comes in the form of a call to the __chkstk function, which just reads every 4Kth byte from the previous stack top to the new stack top, ensuring that all of the necessary stack-reserved pages have actually been committed. (If there is no more memory available to commit the pages, then __chkstk fails.)

So, in this case, the compiler would generate prologue code like the following:

Main PROC
    mov   eax, 1048h
    call  __chkstk
    sub   rsp, rax
    ; ...

See also: Allocating a buffer of more a page size on stack will corrupt memory? and What is the purpose of the _chkstk() function? on Stack Overflow.

Peephole Optimization

• Don't use LEA when MOV will do. The mnemonic suggests that LEA is the way to load an address, and indeed it will do that, but so will MOV, as long as you use the OFFSET operator. Substitute:

  lea  var0, filePath
  

  with

  mov  var0, OFFSET filePath
  

  Honestly, the biggest use of LEA in assembly code is as a fancy way to do general-purpose integer arithmetic on non-address values, since it can perform addition with multiple operands, add and scale (by limited powers of two), simulate a three-operand instruction, and not clobber the flags. You will need it for scaled loads of addresses, but again, there, you're using its fancy address-calculation machinery, not simply to load an offset.

• Use the non-volatile registers (RBX, RBP, RDI, RSI, and R12 through R15) to store temporary values that you need to persist across function calls. The calling convention requires the contents of these registers to be persisted across calls, so anything you have in them will be safe. This abundance of registers on x64 allows you to avoid storing to the stack, and gain a bit more speed. So, replace:

  mov var5, rax                        ; save a reference to the file handle for later (taking advantage of the unused parameter slot 5)
  

  with something like:

  mov  r15, rax
  

• there seemed to be some conflicting information about the cmov instruction and I still don't quite understand if I "should've" gone with an alternative that involves a jump or two.

  I'm not sure what the conflicting information was, but I can make it pretty simple for you:

  • If the branch is predictable, then use an actual branch (jump). That will be faster (less overhead). Trust the branch predictor to do its job.
  • If the branch is not predictable (because you have completely random input, or because it oscillates back and forth), then it is probably better to write branchless code using something like CMOVcc or SETcc. This will avoid the possibility of branch misprediction, at the cost of executing additional code each time.

  That's a good enough rule of thumb. Correctly-predicted branches are virtually free; mispredicted branches are extremely slow (relatively speaking). If you want more information, see this answer.

  Also keep in mind, though, that it is generally pointless to optimize code that is not a performance hotspot (like an inner loop). The branching code is shorter and simpler to write, so that should be your first instinct.

  Branching code is especially appropriate for error handling code like what you have here, since:

  • Error handling is (almost) never a performance hotspot, and
  • Errors should be rare occurrences, and thus the direction of execution in an error check will be correctly predicted by the CPU's built-in branch predictor.

  In fact, optimizing C and C++ compilers even go so far as to explicitly put error-handling code on a cold path, and there are annotations you can use to request this if the optimizer isn't smart enough to do it on its own.

  Your current code is something like:

      call   ReadFile
      mov    rcx, var6     ; rcx <= count of bytes read
      mov    edx, TRUE     ; assume success
      test   eax, eax      ; read failed?
      cmovz  edx, eax      ; edx = ((eax == 0) ? 0 : edx
      test   ecx, ecx      ; reached EOF?
      cmovz  edx, ecx      ; edx = ((ecx == 0) ? 0 : ecx
      test   edx, edx      ; read succeeded and not at EOF?
      jnz    readFileBody  ; if so, keep reading
   readFileFooter:
  

  …wait—is that even right? Stop and write a spec:

  • If read operation failed (ReadFile returns 0 in EAX), stop and fall through to close the handle.
  • If there are no more bytes to read (we've reached the EOF, indicated by the byte count in ECX being 0), stop and fall through to close the handle.
  • Otherwise, keep looping and reading bytes from the file body.

  In C:

  if   ((eax == 0) || (ecx == 0))  { break;    }
  else                             { continue; }
  

  Right? Well, let me first take my own advice and write it as a series of branches:

     call   ReadFile
     mov    rcx, var6       ; rcx <= count of bytes read
     test   eax, eax        ; read failed?
     jz     readFileFooter  ; if so, abort
     test   ecx, ecx        ; reached EOF?
     jnz    readFileBody    ; if not, keep reading
  readFileFooter:
  

  That's extremely simple and easy to understand. It's also going to be quite efficient, because branch prediction is going to be on your side.

  If you insisted upon writing it branchlessly, the first draft would be something like:

     call   ReadFile
     mov    rcx, var6     ; rcx <= count of bytes read
     test   eax, eax      ; \ dl = 1 if read failed;
     setz   dl            ; / dl = 0 if read succeeded
     test   ecx, ecx      ; \ dh = 1 if reached EOF;
     setz   dh            ; / dh = 0 if not at EOF
     or     dl, dh        ; \ if read succeeded and not at EOF,
     jnz    readFileBody  ; /  keep reading; otherwise, bail
  readFileFooter:
  

  That is similar to your CMOVcc code, except that it uses TESTcc instead. It elides one instruction, but does risk a performance decrease on some microarchitectures where partial registers are not renamed separately (because we're using DL and DH, the two byte-accessible portions of the EDX register). We could solve this problem by rewriting as:

     call   ReadFile
     mov    rcx, var6     ; rcx <= count of bytes read
     test   eax, eax      ; \ dl = 1 if read failed;
     setz   dl            ; / dl = 0 if read succeeded
     test   ecx, ecx      ; \ al = 1 if reached EOF;
     setz   al            ; / al = 0 if not at EOF
     or     al, dl        ; \ if read succeeded and not at EOF,
     jnz    readFileBody  ; /  keep reading; otherwise, bail
  readFileFooter:
  

  This clobbers EAX (or, at least, the low 8 bytes of it), but we don't care at this point, because we've already gotten the information we need from the ReadFile function's return value. It still isn't extremely performant code, though. The SETcc instructions have a relatively high latency, and there is a long dependency chain in these instructions. Your original CMOVcc version isn't much better on either of those fronts. I personally find the SETcc version more idiomatic and therefore more readable, but not strongly so; choose whichever you like better.

  Your revised CMOVcc version is better. Or, at least, it is shorter. Whether it's actually better is a matter for a profiler to decide. Shorter code is not necessarily "better" or faster.

     call  ReadFile
     mov   ecx, -1       ; NOTE: prefer to write constant as -1, not 0FFFFFFFFh
     test  eax, eax
     cmovz ecx, eax
     and   rcx, var6
     jnz   readFileBody
  readFileFooter: