4
\$\begingroup\$

I recently posted a code review and received some very useful feedback. I have spent some time implementing the suggested improvements/fixes, and wish to post a follow up review, with the new and improved version of the program.


Changes Made:

Comments Section(s):

I have added an additional ‘Register Use’ section to the comments block, at the header of the main program. The section describes how the main registers are used through-out the main program. For information regarding individual procedures/macros, please see the relevant header sections.

In addition, I have taken onboard the feedback with regards to line length. Originally, some lines of source code were in excess of 200 columns. All lines now wrap at a column count of 80.

Fix the Bug - One:

The main fault with the program was that it had a fatal bug. In short, the program had originally attempted to create the entirety of its output in a relative small memory reserve of 64K. The fix was given in the accepted answer to the original question. By handling the data one line at a time, the program would only require a small memory reserve, as each line of data would be written to the console, prior to the subsequent read from file. This also had the added benefit of reducing the amount of necessary data processing, as I was able to declare the output line formatting (vertical bars etc.) in memory, as data, as opposed to implementing the formatting by way of code.

As a general note, the reason I chose not to structure the code in this manner in the first instance, was because I was attempting to write the most efficient code possible. My concern was that, by handling data one line at a time, I would unnecessarily be increasing the number of system calls, in this instance sys_write calls, and consequently my program would be slower overall. Ultimately though, efficiency is not of significance, if the program does not work to begin with!

Fix the Bug – Two:

Procedure 'CharToHex' had originally assumed that the high bytes of EAX and EBX were zero (thanks to @1201ProgramAlarm). Whilst looking up the individual nibbles of the input chars, in lookup-table 'Digits', if the high bytes were in fact nonzero, then the procedure may have ended up referencing memory out of bounds. Or, at the very least, it would most certainly have not been pointing to the correct table address. The fix was to clear both registers at the start of the procedure, using XOR.

Measurements / Testing:

I tested the program, both before and after the restructure, using the strace utility. My prediction was that by handling the data one line at a time, the program would be notably slower; this has to be the case, as system calls can not have a zero cost.

I measured the average execution time, by running each version of the program through strace ten times, recording each result, and then by taking the average of the ten measurements. N.B. I appreciate my measurements are somewhat crude.

The results show that on average, the program, as originally constructed, took 0.0199 seconds to execute. Using the same test data, the average time taken for the program to execute, after the restructuring (i.e. handling data one line at a time), was 0.0700 seconds. Therefore, handling the data one line at a time does indeed slow down the program. The program is now, roughly, three and a half times slower. This is as expected.

Avoid Slow Instructions:

I have significantly changed the procedure ‘ConvertControlChars’. There were two main reasons to do so. Firstly, the procedure originally made use of a translation table, to convert any non-printable ASCII characters in the ‘InputBuff’ string, to printable ASCII period characters (2Eh). To implement this functionality, the program utilized the XLAT instruction, in order to scan the string and convert any relevant chars. As pointed out in the original review, XLAT is a relatively slow instruction, Agner Fog's instruction tables, show that this is indeed in the case. XLAT also demands the use of implicit operands/registers, which can be somewhat limiting, and can require additional processing, if the demanded registers are already in use.

The same result is now achieved through the implementation of a conditional loop. The loop scans the ‘InputBuff’ string, and by using the CMP instruction, decides on whether or not the char requires conversion. The outcome is that the size of the procedure has been reduced from a total of 16 instructions, to 12. Moreover, it avoids the use of the relatively slow XLAT instruction.

The translation table, PeriodXLat, has also been removed, as it is surplus to requirements. This reduces the overall program size by a minimum of 256 bytes; the number of declared bytes in the table.

Use Fast Instructions:

The section of code, that handles the processing of the output line padding, as also been restructured. I have reduced the necessary instruction count from 16 to 7, a significant saving. This has mainly be achieved by removing the conditional loops ‘CharPadding’ and ‘RowBuffer’, and replacing them with a simple STOSB instruction.

Use of the Stack

I have reduced the total number of push/pop instructions by 8.

Be wary of using RBP as a data pointer:

The original program made heavy use of the EBP register as a data pointer. As kindly pointed out, this is not generally good practice, as its usual use is as a stack frame pointer. Using the register in this manner can lead to potential issues. The program now uses EBX as a pointer instead.

Rethink Error Handling

Error messages are now written to stderr as opposed to stdout, as can be seen in the changes made to the ‘ErrorHandler’ macro. I have also updated the header section of this particular macro, to more accurately reflect the macros purpose and use of registers.


Changes Not Implemented:

Use Memory Efficiently:

One suggested improvement, was to eliminate the necessity of separate input and output buffers, by reading data directly into place, and carrying out the necessary processing from within the one memory buffer. However, I do not think that this is possible, as for each byte read from file, two bytes are written to stdout. Attempting to convert the characters in situ, would mean overwriting the second character, during the conversion of the first, and so on and so forth. For example, when reading char “A” from file, the underlying binary stored in memory, is 41h, one byte. In order to print “41” to the terminal, the single byte read from file, is converted to two bytes, 3431h. If the conversion was done ‘in place’, the byte 31h would overwrite the second byte read from file, before it had been processed.


Review Request:

I believe my program is now bug free. I have tested it more thoroughly this time, using much larger files. Again, though, I would very much appreciate someone with the requisite knowledge pointing out my ignorance, if I happen to be incorrect.

I would also appreciate a general critique; what areas of code would you consider still require improvement; what fine-tuning, if any, can still be made; and, importantly, are there any particular areas where you would have done things differently, and why.

Ultimately, I am trying to learn as much as possible, text books etc. are useful, but I have no way of knowing it I am internalizing/implementing the teachings correctly; I have no test. I would very much appreciate a final critique. What advice can you give, with regards to what I should be thinking about going forward, prior to embarking on my next project.


Notes:

i. In my project, macros are treated as %include files, and procedures are assembled separately into their own object files. All modules have been assembled as one for the purposes of posting this review request.

ii. NASM version 2.11.08 | Architecture x86-64 | Ubuntu 18.04


; Executable name: hexdumpadvanced
; Version        : 1.1
; Creation date  : 22/08/2018
; Last updated   : 22/08/2018
; Author         : Andrew Hardiman
; Architecture   : x86-64
; Register Use   : Registers in the main program are used as follows: EBX is 
;                  used as a pointer to memory data, specifically to offset 
;                  `InputBuff`; ECX is used as a counter register, for example 
;                  it is used within the main program loop `ReadFile`, to count 
;                  the number of passes through the loop itself; EDI is used to 
;                  store the destination memory offset, when moving, or storing 
;                  data to memory. For example, EDI is used as a pointer to the 
;                  string 'OutputHex', the output string of the main program.  
;                  For register usage within specific procedures/macros, please 
;                  see the relevant procedure/macro heading section.
; Description    : A hex dump utility. The program reads data from stdin and 
;                  and converts the input to rows of hexadecimal pairs, 
;                  representing the underlying binary notation of the data.
;                  The hex editor also displays the related ASCII chars 
;                  alongside each row of hexadecimal pairs. The length of the 
;                  program output, i.e. how many hex-pairs per row in the 
;                  terminal is dictated by the constant 'INPUTLEN'. NB If 
;                  changing 'INPUTLEN' strings 'OutputHex' and 'OutputChars' 
;                  will need to be changed also, to match.
; Macros         : The program includes macro files: "system_call_macros" and 
;                  "string_macros".
; Procedures     : There are two externally linked procedures: 'CharTohex.o' 
;                  and 'ConvertControlChars.o'. 
;  
; Run with the following commands:
;   ./hexdumpadvanced < [Input_File]
;
; Build with the following commands:
;   ld -o hexdumpadvanced hexdumpadvanced.o
;   nasm -f elf64 -g -F dwarf hexdumpadvanced.asm

SECTION .data                          ; initialised data 

; A lookup table for use with procedure 'CharToHex':
    Digits:        db "0123456789ABCDEF"            

; Error message to stderr when an error code is returned from kernel `syscall`:
    ErrorMSG:      db "There has been an unexpected error, your program has" 
                   db " terminated",0Ah         
    ERRORLEN:      equ $-ErrorMSG

; Message printed to stdout, when the file passed to stdin contains no data:
    ZeroInput:     db "The input file did not contain any data, the program" 
                   db " has terminated",0Ah     
    ZEROLEN:       equ $-ZeroInput

; Predefined output buffer, to recieve the input chars converted into their 
; binary notation, and the ASCII chars, delimited by vertical bars (7Ch): 
    OutputHex:     db "00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 "     
    OutputChars:   db "|................|",0AH                               
    OUTPUTLEN:     equ $-OutputHex

SECTION .bss                            ; uninitialised data 

    INPUTLEN:      equ 16               ; Read from file, 16 bytes at a time
    InputBuff:     resb INPUTLEN

SECTION .text                           ; section containing code

;-------------------------------------------------------------------------------
; MACROS START HERE
;-------------------------------------------------------------------------------

;-------------------------------------------------------------------------------
; ReadInput   : Invokes x86-64 sys_read. Kernel `syscall` no. 0
; Updated     : 19/08/2018
; IN          : %1 is the memory offset to read to; %2 is the byte count
; Returns     : RAX will contain the number of bytes read to memory
; Modifies    : RAX as the return value; R11 is clobbed with the value of RFLAGS
; Calls       : Kernel `syscall`
; Description : ReadInput simplifies invoking kernel `syscall` in x86-64, 
;               specifically for `syscall` number 0; sys_read. The macro 
;               preserves and restores the callers registers.

%macro ReadInput 2                                  
; Save callers registers.   
    push rcx          ; kernel syscall stores RIP in RCX    
    push rdx          ; Used to store the read byte count
    push rdi          ; Stores file descriptor, stdin '0'
    push rsi          ; Memory offset to read file

; Prepare registers, and invoke kernel sys_read:
    mov eax,0         ; sys_read
    mov edi,0         ; stdin
    mov esi,%1        ; Memory offset to read to
    mov edx,%2        ; Byte count read from file
    syscall           ; Kernel system call

; Restore callers registers:
    pop rsi
    pop rdi
    pop rdx
    pop rcx
%endmacro


;-------------------------------------------------------------------------------
; WriteOutput : Invokes x86-64 sys_write. Kernel `syscall` no. 1
; Updated     : 19/08/2018       
; IN          : %1 memory offset delimiting the start of data to write to 
;               output; %2 number of bytes to write to output.
; Returns     : Possible error code to RAX
; Modifies    : RAX possible error code; R11 is clobbed with the value of    RFLAGS
; Calls       : Kernel `syscall`
; Description : WriteOutput simplifies invoking kernel `syscall` in  x86-64, 
;               specifically for `syscall` number 1; sys_write. The macro 
;               preserves and restores the callers registers.

%macro WriteOutput 2                                
; Save callers registers. RAX will be clobbered with `syscall` return code:
    push rcx          ; Kernel `syscall` stores RIP in RCX
    push rdx          ; Byte count to write to stdout
    push rdi          ; File descriptor 1, stdout
    push rsi          ; Offset of data to written to stdout

; Prepare registers, and invoke kernel `sys_write`:
    mov eax,1         ; sys_write
    mov edi,1         ; stdout
    mov esi,%1        ; Offset of data written to stdout
    mov edx,%2        ; Number of bytes written to stdout
    syscall           ; Invoke kernel `syscall`. 

; Restore callers registers:
    pop rsi
    pop rdi
    pop rdx
    pop rcx
%endmacro


;-------------------------------------------------------------------------------
; ExitProgram : Invokes x86-64 sys_exit. Kernel `syscall` no. 60
; Updated     : 19/08/2018
; IN          : Nothing
; Returns     : Return code to RAX
; Modifies    : RAX contains return code; RDI int error_code (typically) zero; 
;               RCX stores RIP, R11 store RFLAGS.
; Calls       : Kernel `syscall`
; Description : Exits program elegantly and hands control back to the kernel 
;               from user space; probable segmentation fault without invocation 
;               of kernel sys_exit.

%macro ExitProgram 0
; Prepare resgiters, and invoke kernel sys_exit:
    mov eax,60        ; Kernel syscall no. 60, sys_exit
    mov edi,0         ; Return error code 0
    syscall
%endmacro


;-------------------------------------------------------------------------------
; ErrorHandler : Displays error message to stderr and exits program elegantly
; Updated      : 21/08/2018
; IN           : To be included in `SECTION .data` of main program: `ErrorMSG: 
;                db "There has been an unexpected error, your program has 
;                terminated"` and `ERRORLEN: equ $-ErrorMSG`. 
; Returns      : RAX will contain the return code from `sys_exit` kernel call
; Modifies     : RAX will contain the return code from `sys_exit` kernel call; 
;                RDI will be 0; RCX stores RIP, R11 stores RFLAGS; RSI will be 
;                the memory offset of string 'ErrorMSG'; RDX will be the byte 
;                count of string 'ErrorMSG', stored in label 'ERRORLEN'. 
; Calls        : Includes 'ExitProgram' macros, from file "system_call_macros"
; Description  : To be invoked after a `syscall`, to check RAX for an error 
;                return code. Under Linux, error return codes are within the 
;                range -4095..... -1. If error code returned from `syscall`, 
;                error message is written to stderr and program exits 
;                through 'ExitProgram' macro.

%macro ErrorHandler 0
    cmp rax,0FFFFFFFFFFFFF000h ; Error range under Linux is -4095 ...... -1
    jna %%exit                 ; Return value > RAX indicates error

; Write error message to stderr:    
    mov eax,1                  ; Kernel syscall no. 1, sys_write
    mov edi,2                  ; File descriptor 2, stderr
    mov esi,ErrorMSG           ; Offset of string to write to stderr
    mov edx,ERRORLEN           ; Length of message to write to stderr
    syscall                    ; Make system call

; Exit program elegantly:    
    ExitProgram                ; ExitProgram macro

%%exit:
%endmacro


;-------------------------------------------------------------------------------
; MoveString        : Moves string from memory offset A to memory offset B
; Updated           : 19/08/2018
; IN                : %1 is the destination memory offset; %2 is the source 
;                     memory offset; %3 is the byte count in the string.
; Returns           : Nothing
; Modified/Trashed  : EDI will point to memory offset immediately after the 
;                     last char in the moved string.
; Calls             : Nothing
; Description       : The macro is used to invoke the instruction 'rep 
;                     MOVSB', it is useful as it preserves registers and 
;                     reduces necessary key-strokes.

%macro MoveString 3                            
    push rcx
    push rsi

    lea edi,%1                ; Destination memory address for `MOVSB`
    lea esi,%2                ; Source memory address for `MOVSB`
    mov ecx,%3                ; The byte count of the string being moved
    rep movsb                                   

    pop rsi
    pop rcx
%endmacro


;-------------------------------------------------------------------------------
;PROCEDURES START HERE
;-------------------------------------------------------------------------------

;-------------------------------------------------------------------------------
; CharToHex   : Converts a string of chars in memory, to their underlying 
;               binary representations, see Description
; Architecture: x86-64
; Updated     : 21/08/2018
; IN          : EBX is the memory offset of the string of input chars; EDI is 
;               the memory offset of the string of converted converted 
;               hexidecimal pairs; ECX is the number of chars to convert.
; Returns     : Hexidecimal pairs are stored at memory offset EDI
; Modifies    : EDI will point to the memory offset immediately after the last 
;               hex-pair stored in memory; ESI will contain the delimiter 
;               character passed to 'CharToHex' as an original argument. 
; Calls       : Nothing
; Description : CharsToHex excepts a string of ASCII chars, at offset EBX, and 
;               converts the chars to a string of chars representing their 
;               underlying binary representations, to memory offset EDI. For 
;               example, if char at EBX was "A", then [EBX] would contain the 
;               underlying binary notation 41h. CharsToHex would then generate 
;               a string at EDI representing the chars "4" and "1" (binary in 
;               memory 3431h). Consequently, when the input is "A", the output 
;               is "41"; the output is the underlying hexidecimal notation of 
;               the input.

CharToHex:
    push rax        
    push rbx
    push rcx
    push rdx

; During loop `.convertChars`, if the high byte of registers AX and DX are not 
; zero then, the use of `[Digits+eax]` could end up accessing data out of 
; bounds of memory. This would be in the case, for instance, if the number of 
; chars to be converted, EAX, exceeded 255 characters: 
    xor eax,eax                             
    xor edx,edx

.convertChars:
    mov al,byte [ebx]          ; Move byte from input buffer to AL
    mov dl,al                  ; Copy char into DL
    and al,0Fh                 ; Bit-mask, AL will now hold lower nibble
    shr dl,4                   ; DL will now hold upper nibble of hex-pair

;Look up nibble in lookup table 'Digits', return the underlying binary pattern:
    mov al,byte [Digits+eax]   ; Lookup digit in 'Digits' table
    mov dl,byte [Digits+edx]   ; Return the underlying binary notation
    mov byte [edi],dl          ; Move binary pattern to Output string
    mov byte [edi+1],al        ; Move binary pattern
    mov byte [edi+2],20h       ; Append 'space' character to output string
    lea edi,[edi+3]            ; Move output pointer
    inc ebx                    ; Increment input buffer pointer
    dec ecx                    ; Decrement the count of chars 
    jne .convertChars          ; If char count not zero, convert next char

; Restore registers and return:
    pop rdx
    pop rcx
    pop rbx
    pop rax
    ret 

;-------------------------------------------------------------------------------
; ConvertControlChars   : Converts a string of chars in memory, replacing 
;                         non-printable chars with the ASCII period character, 
;                         2Eh; printable characters are left unchanged.
; Architecture          : x86-64
; Updated               : 22/08/2018
; IN                    : RCX is the length of the string being scanned, in 
;                         bytes; RBX is the pointer to the offset of the string 
;                         being scanned. 
; Returns               : Nothing
; Modifies              : Nothing, any registers modified during the procedure 
;                         are reserved on the stack, and are restored prior to 
;                         returning to the main program.
; Calls                 : Nothing
; Description           : Scans a string of chars in memory. The high 128 
;                         characters are translated to ASCII period (2Eh). 
;                         The non-printable characters in the low 128 
;                         (00h -1Fh) are also translated to ASCII period, as is 
;                         char 127 (7Fh).

ConvertControlChars:
; Preserve registers:
    push rax
    push rcx

; Convert string of ECX length, starting at offset EBX:
.nextChar:
    mov al, byte [ebx-1+ecx]   ; Move first char for conversion to register
    cmp al,20h                 ; Compare char in string to 20h
    jb .convertChar            ; Chars below ASCII 20h are non-printable
    cmp al,7Eh                 ; Compare char in string to 7Eh
    jna .testExit              ; chars above 7Eh are non-printable

.convertChar:
    mov byte [ebx-1+ecx],2Eh   ; Char has tested positive as non-printable

.testExit:
    dec ecx                    ; Decrement count of chas to be converted 
    jnz .nextChar              ; Loop if there are chars remaining

; Restore registers and return:
    pop rcx
    pop rax
    ret


;-------------------------------------------------------------------------------
; MAIN PROGRAM STARTS HERE
;-------------------------------------------------------------------------------

GLOBAL _start                      ; Linker need this to find an entry point  

_start:
    nop                            ; This no-op keeps gdb happy....

; Create a pointer, for the 'InputBuff' memory buffer. The instruction is 
; situated here in the source code, as the instruction does not need to be 
; repeated each time the program loops: 
     lea ebx,[InputBuff]

; Read data from stdin, to memory offset 'InputBuff':    
ReadFile:
    ReadInput InputBuff, INPUTLEN  ; Macro 'ReadInput' 
    ErrorHandler                   ; Macro 'ErrorHandler'

; EDX will store the aggregate number of bytes read from file:
    add edx,eax                

; Check return value from sys_read. If no data has been read, and the program 
; is on first loop (EDX), then there has been no data read from file, inform 
; user and exit program. If there has been data read from file (EDX), however 
; there is no data read on this loop (EAX), then EOF reached, 'PrintOutput' and 
; exit program:
    cmp eax,0                      ; Compare sys_read return value to zero
    jne InputPadding               ; If data has been read, jp to 'InputPadding' 

    cmp edx,0                      ; Compare loop count to one
    jne Exit                       ; If data has previously been read,jp 'Exit'

; Inform user that no data has been read and exit program:
    WriteOutput ZeroInput, ZEROLEN ; Macro 'WriteOutput'            
    ErrorHandler                   ; Macro 'ErrorHandler'          
    jmp Exit                       ; Jump to 'Exit' (program)

; If the number of bytes read from file < 'INPUTLEN', calculate the padding 
; required to overwrite the bytes 'left-over' from the previous loop. This step 
; is required to prevent the program from writing duplicate data on the 
; final line of output in the terminal:
InputPadding:
    cmp eax,INPUTLEN               ; cmp INPUTLEN to no. of chars read from file 
    je ConvertChars                ; If INPUTLEN == chars from file: no padding

; Prepare implicit registers for use with `STOSB` instruction:
    mov ecx,INPUTLEN               ; Move maximum number of bytes read from file
    sub ecx,eax                    ; Subtract the actual number of bytes read
    lea edi,[ebx+eax]              ; Offset in which to store the string
    mov eax,0h                     ; Move character to use as padding

; Execute store string instruction, which will overwrite any 'left-over' bytes, 
; from previous `sys_read`:
    rep stosb                      ; Reiterate through string

; Convert each individual char, in 'InputBuff', to a string representing its 
; underlying binary notation, and store at memory offset 'OutputHex'. For 
; example, if char in memory is "A", the underlying binary notation will be 
; 41h. Therefore, 'ConvertChars' will create the word 3431h, in memory 
; buffer 'OutputHex'. 3431h printed to stdout will be converted to string 
; "41", the binary notation of char "A":
ConvertChars:
    lea edi,[OutputHex]            ; Create pointer for 'OutputHex' stringFor
    mov ecx,INPUTLEN               ; The number of chars to convert
    call CharToHex                 ; Convert ASCII chars read from file, to
                                   ; their underlying binary notation

; Convert all non-printable chars in 'InputBuff' to period character (2Eh):
    call ConvertControlChars                   

; Move string of chars from 'InputBuff', to relevant place in 'OutputChars' 
; string, accounting for the 'opening' vertical bar, with '+1'.
; A row of chars will appear immediately after the row of related hex-pairs in 
; the terminal output, 'book-marked' either end by a vertical bar char (7Ch):
    MoveString [OutputChars+1], [InputBuff], INPUTLEN

; Print the prepared output string to the terminal, delimited at the offset in
; memory `OutputHex`. The string consists of two `SECTION .data` items, 
; 'OutputHex', containing the hexidecimal pairs, and `OutputChars`, containing 
; the ASCII chars, with all control chars converted to the period character 2Eh.
; The total output string is of length `OUTPUTLEN`:
PrintOutput:
    WriteOutput OutputHex, OUTPUTLEN ; Macro `WriteOutput`        
    ErrorHandler                     ; `ErrorHandler` macro

; Fetch next buffer of input from file and repeat the process:
    jmp ReadFile

; Exit program elegantly:
Exit:
    ExitProgram                      ; 'ExitProgram' macro

    nop                              ; no-op keeps gdb happy.......
\$\endgroup\$
3
\$\begingroup\$

I see a number of things that may help you improve your program, but first I wanted to say that you've definitely improved the program greatly from the previous version. Nice work!

Fix the (minor) bug

The previous version correctly showed blanks at the end of the lines if there were fewer actual bytes in the file than the line length called for. Unfortunately, that's no longer the case, so this command:

echo "foo" |hexdumpadvanced 

Produces this response:

66 6F 6F 0A 00 00 00 00 00 00 00 00 00 00 00 00 |foo.............|

That's not really correct because there are only 4 bytes and not 16. The response of the previous version is what I would expect:

66 6F 6F 0A                                     |foo.            |

Use shorter instructions

Since we're writing in assembly language, it makes sense to use that to our advantage. One way to do that is to keep the code as small as possible to help facilitate caching. If the code is small enough to fit entirely within L1 cache, for example, (and this program is), it won't have to go back out to slower memory (L2, L3 caches and hard drive) as often. One way to keep the code small is by using smaller instructions. One could write, for example,

mov rdi, 0    ; this sets edi to zero with a 5-byte instruction
xor rdi, rdi  ; so does this but with a 3-byte instruction
xor edi, edi  ; so does this but with a 2-byte instruction

The difference, however, is that the xor instructions are two and three bytes shorter. They also set the flags register (unlike a mov) so don't use it if you need to preserve flags. There are quite a number of places in the code where that particular trick can be used.

Don't save more registers than needed

Most of the skill involved in assembly language programming, and especially for the x86 series processors, is in making very careful use of registers. So while it's often a good idea to save used registers (as your program generally does), careful use of registers while avoiding pushing and popping is how to make programs smaller and faster. While the use of macros tends to make the program easier to read and understand, it can also hide the fact that you're generating inefficient code. Use nasm's -l flag to generate a program listing. It will show you exactly what machine language bytes are generated and you will find, for example, that the program pushes and pops the rcx many, many times and for no good reason since every time the rcx register is needed, it's explicitly set to the required value anyway.

Use memory efficiently

Right now, there is an input area and an separate output area, but they could easily be combined. Consider the output data format for an 8-byte/line output:

00 01 02 03 04 05 06 07 |........|
                         ^
                     InputBuff

As shown above, the input buffer could be part of the output line. That is, read the data into the location shown (part of the output buffer), convert to pairs of hex bytes to the left and then convert in-place to replace non-printable characters with '.' characters.

Understand standard elf segments

The .data section is a writable area that is initialized by reading data from the file. However, there is also an .rodata which is read-only data. That would be the more appropriate segment to use for the error messages, since there's no reason to modify those at runtime.

Restore the ability to tune the output

One of the things that was good about the previous version that is lost in this one is that one could simply redefine the INPUTLEN constant and recompile to change the line size. This doesn't need to be the case, as I'll demonstrate in a rewritten version below.

Make jumps as short as possible

Shorter jumps are generally faster than longer jumps, so keeping the code compact helps assure that the jumps are short. Of course the shortest jump is no jump at all, so eliminating jumps entirely is even better for performance because not only does it make the code shorter, but it also tends to eliminates cache misses and pipeline stalls.

Eliminate subroutine calls for maximum performance

If a routine is only used one place, it's generally faster to have the code in line rather than using a call.

Use buffering for read/write or both

C and C++ tend to use buffered reads and writes for performance. It's also quite possible to do in assembly language and has significant performance benefits. One way to think about it: write a subroutine that mimics a sys_read kernel call in that the buffer to write is in rsi, and the desired length is in rdx. Then it would return either an error code or the number of bytes actually read in rax. For the first iteration, it could simply be a wrapper for the sys_read kernel call. Then enhance it by adding a buffer. I'll show a worked example in the code below, but it might be a better learning experience if you try to implement it yourself before looking at my version. You might also want to implement write buffering for further performance gains.

Consider adding an offset

The dump would contain more information if each line started with the hexadecimal offset of that portion of the file.

Consider other user enhancements

It would be nice to be able to do things like dynamically control the length of the lines via a command-line switch. It would also be nice to allow for turning a hex dump back into a binary file. For further inspiration, look at the man page for xxd and test its performance against your program.

Measured performance

I used a single video file with a length of 5144214118 bytes (5 Gib) to do some performance testing. The earlier version of your program crashed with files over 64Kib in length, so I wasn't able to time that. However, here are the timings for both your program and the updated one posted below with various settings. With this large file, output was sent to /dev/null because I didn't want to use so much disk space.

3:45.09 version 1.1  
2:17.00 updated with no address, 16Kib buffer
4:11.07 updated, no address, no buffering
2:21.48 updated with 8-byte address, 16Kib buffer

As noted above, read buffering provides a significant performance gain. I also added the feature of showing the address (offset) of each line. Finally, note that the three basic settings of address size, line size and buffer size are all (mostly) independently settable, so one can even specify weird things like a 3-byte address size and a 7-byte line size if desired.

Updated version

; hexdump.asm
;
; This program reads INPUTLEN bytes at a time from stdin and writes out a 
; formatted hex version as a dump.  For an INPUTLEN of 8 and ADDRBYTES = 4,
; for example, here is an example of an output line:
;
; 00000000: 66 6F 6F 0A             |foo.    |
; 
; Very little error checking is done, because if writing to stdout fails, 
; what would we do anyway?  An error on reading from stdin causes the 
; program to exit with the received error code.  An input of zero length
; is not considered an error and simply produces no output.
;

SECTION .bss
;----------------------------------------------------------------------
; Configurable settings
;----------------------------------------------------------------------
    ; ADDRBYTES is the size of the displayed address counter in bytes
    ; set it to zero to omit display of addresses or to 8 for max size
    ADDRBYTES: equ 4
    ; INPUTLEN is the number of data bytes to display per line
    INPUTLEN: equ 16
    ; BUFFERBYTES is the size of the input buffer used or set to zero
    ; to disable buffering.  This must always be some multiple of
    ; INPUTLEN
    BUFFERBYTES: equ INPUTLEN * 1024

;----------------------------------------------------------------------
; Derived settings
;----------------------------------------------------------------------
    ; ADDRLEN includes the length of the address (8 hex digits) 
    ; plus the ": " immediately following
%if ADDRBYTES
%if ADDRBYTES > 8
%error "ADDRBYTES must be an integer in the range 0 through 8 inclusive"
%endif
    ADDRLEN: equ ADDRBYTES * 2 + 2
%else
    ADDRLEN: equ 0
%endif
    OUTPUTLEN: equ INPUTLEN * 4 + 3 
    Address: resb ADDRLEN
    Output: resb OUTPUTLEN      
    InputBuff: equ Output + (INPUTLEN * 3) + 1  
%if BUFFERBYTES
%if BUFFERBYTES % INPUTLEN
%error "BUFFERBYTES must be an integral multiple of INPUTLEN"
%endif
    ReadBuff: resb BUFFERBYTES
    remaining: resq 1
    current: resq 1
    noMore: resb 1
%endif

SECTION .text
;----------------------------------------------------------------------
; ALtoHex
;   converts the input byte in al to two ASCII hex digits in
;   al,ah as hi,lo digits so that STOSW may be used to store
;   them in the correct order
;
; INPUT: al = input byte to be converted to ASCII hex digits
; OUTPUT: al,ah = hi,lo ASCII hex digits 
; TRASHED: all other bits of RAX are zeroed
; 
;----------------------------------------------------------------------
%macro ALtoHex 0
        mov ah, al                  ; 
        shr al,4                    ; 
        and eax, 0f0fh              ; now hi and lo nybbles in ah,al
        add eax, 3030h              ; convert to ASCII digits
        cmp al,'9'                  ; high digit in printable range?
        jbe %%chkLoDigit             
        add al, 7                   ; if not it must be A-F, so add 7 
%%chkLoDigit:
        cmp ah, '9'                 ; lo digit in printable range?
        jbe %%writeHexPair
        add ah, 7                   ; if not it must be A-F, so add 7 
%%writeHexPair:
%endmacro

;----------------------------------------------------------------------
; main routine
;   writes formatted hex dump of STDIN to STDOUT
;
; Register usage:
; rax - various purposes, mostly data
; rbx - address (preserved across system calls)
; rcx - used as count of various things
; rdx - length of most recently read "line"; <= INPUTLEN
; rdi - destination pointer, various
; rsi - source pointer, various
; 
;----------------------------------------------------------------------
GLOBAL _start                                      
_start:
        xor ebx, ebx                ; initialize address = 0
%if ADDRBYTES
        mov al,'0'                  ; fill address with all zeroes
        lea rdi, [Address]
        mov rcx, ADDRLEN - 2
        rep stosb
        mov al,':'                  ; then ':'
        stosb
        mov al,' '                  ; then ' '
        stosb
%endif
%if BUFFERBYTES
        mov [remaining], rbx        ; clear remaining
        mov [noMore], bl            ; clear no-more file bytes flag
%endif
        ; fill output buffer
        mov al, ' '                 ; fill rest of line with spaces 
        lea rdi, [Output]
        mov rcx, OUTPUTLEN - 2
        rep stosb
        mov al, '|'                 ; then '|'
        stosb
        mov [InputBuff - 1], al     ; another '|' at the end
        mov al, 0ah                 ; and newline char
        stosb
        jmp .readline

.checkLen:
        ; at this point we've just read a line. len in rax and rdx
        mov rcx, INPUTLEN
        sub rcx, rax                ; if it's a short line, remainder in rcx
        jz .tohex
        ; this is a short line so pad areas with spaces
        ; first, the ASCII area
        mov rsi, rcx                ; save pad size in rsi
        mov al, ' '                 ; spaces
        std
        lea rdi, [InputBuff + INPUTLEN - 1]
        rep stosb
        ; now the hex area
        lea rdi, [InputBuff - 2]
        lea rcx,[rsi*2 + rsi]       ; now store three spaces for each byte
        rep stosb
        cld
.tohex:
%if ADDRBYTES
        ; convert address counter into ASCII digits
        lea rdi, [Address]          ; point to output buffer
        mov rsi, ADDRBYTES          ; convert this many bytes
        mov rcx, (ADDRBYTES - 1) * 8  ; shift this many bits
.addrLoop:
        mov rax, rbx                ; recall address
        shr rax, cl
        ALtoHex                     ; macro to convert to ASCII hex
        stosw                       ; store two ASCII hex digits
        sub rcx, 8                  ; next time shift 8 fewer bits
        dec rsi
        jnz .addrLoop
        add rbx, rdx                ; increment address counter
%endif
        ; convert each input byte to two ASCII hex digits
        lea rdi, [Output]           ; 
        lea rsi, [InputBuff]        ;
        mov rcx, rdx                ; recall line length
.hexLoop:
        lodsb                       ; fetch next byte 
        ALtoHex                     ; macro to convert to ASCII hex
        stosw                       ; store two ASCII hex digits
        inc rdi                     ; skip space
        dec rcx                     ; do entire line
        jnz .hexLoop

        ; now convert the input to printable chars
        lea rdi, [InputBuff]
        mov rcx, rdx                ; recover length to rcx
.substLoop:
        mov al, byte [rdi]          ;
        cmp al, 20h
        jb .substitute
        cmp al, 7eh
        jbe .nextchar
.substitute:
        mov byte [rdi], '.'
.nextchar:
        inc rdi
        dec rcx
        jnz .substLoop

        lea rsi,[Address]
        mov edi,1                   ; write to stdout
        mov rdx, OUTPUTLEN + ADDRLEN
        mov rax, rdi                ; sys_write (1)
        syscall

.readline:
        lea rsi,[InputBuff]         ; prepare to read a line of bytes
        mov rdx, INPUTLEN
%if BUFFERBYTES
;----------------------------------------------------------------------
; BufferedRead
;   converts the input byte in al to two ASCII hex digits in
;   al,ah as hi,lo digits so that STOSW may be used to store
;   them in the correct order
;
; INPUT: rsi = buffer into which we read
;        rdx = requested number of bytes
; OUTPUT: rax = actual number of bytes read or -1 on error
; TRASHED: none
; 
;----------------------------------------------------------------------
.BufferedRead:
        ; if remaining < request size, fetch another block
        cmp rdx, [remaining]
        jbe .copyBuffer
        xor eax, eax
        cmp [noMore], al
        jnz .copyBuffer
        ; fetch block
        push rdx
        push rsi
        lea rsi, [ReadBuff]
        mov [current], rsi
        mov rdx, BUFFERBYTES
        xor edi, edi
        xor eax, eax
        syscall
        pop rsi
        pop rdx 
        or  eax, eax                ; was there an error?
        jle .exit                   
        cmp rax, BUFFERBYTES        ; was it a short read?
        adc byte [noMore], 0        ; if so, set flag
        mov [remaining], rax
        ; if remaining >= request size, just copy from buffer
.copyBuffer:
        cmp rdx, [remaining]
        jbe .justCopy
        mov rdx, [remaining]        ; if not enough bytes, use all remaining
.justCopy:
        mov rcx, rdx
        mov rdi, rsi
        mov rsi, [current]          ;
        rep movsb                   ; move bytes
        sub [remaining], rdx        ; decrement remaining
        add [current], rdx          ; advance current pointer
        mov rax, rdx                ; return remaining bytes
.exit:
%else
        xor edi, edi
        xor eax, eax                ; sys_read (0)
        syscall
%endif
        mov rdx, rax                ; save actual len in rdx temporarily
        or  eax, eax
        jg  .checkLen               ; keep going if OK
        mov rdi, rax                ; bail out on error
        mov rax, 60                 ; sys_exit (60)
        syscall

        ; END of program
\$\endgroup\$
  • \$\begingroup\$ "xor rdi, rdi" - Yes, this does zero the 64bit register using only 3 bytes, but so does "xor edi, edi", and it's only 2 bytes (see this or that). \$\endgroup\$ – David Wohlferd Aug 23 '18 at 21:35
  • \$\begingroup\$ @DavidWohlferd: Good point, thanks. I've amended my answer. \$\endgroup\$ – Edward Aug 23 '18 at 21:47
  • \$\begingroup\$ I'm less sure about checking errors with or rax, rax. Doesn't linux typically return 0 to indicate success and -1 for an error? If so, it seems like you could use test eax, eax and save another byte. \$\endgroup\$ – David Wohlferd Aug 23 '18 at 21:57
  • \$\begingroup\$ @DavidWohlferd: I think you'll find that or and test have the same effect in the cases it's used here. Linux returns a negative number on error or the number of bytes read (for sys_read) or written (for sys_write). \$\endgroup\$ – Edward Aug 23 '18 at 22:44
  • \$\begingroup\$ Yes, 'or' and 'test' are the same length/duration. But 'or rax, rax' is not the same as 'or eax, eax.' Seems like either should suffice when checking for -1. \$\endgroup\$ – David Wohlferd Aug 23 '18 at 23:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.