6
\$\begingroup\$

As a relative newbie to Assembly programming, although I can get my programs to work, I have no idea if they are disastrously constructed or not.

I have created a simple Hex Dump Utility, using x86-64 under Linux, and it works just fine!

I am learning in isolation however, so I have no peers, or mentors/teachers, to berate me when I implement poor coding choices, overlook the obvious, or just display plain ignorance.

My Concerns:

As stated, the code works fine. I have tested it (relatively) thoroughly, and the outcome is always as expected.

I would particularly appreciate input with regards to the following:

  1. Although I cannot see any bugs with the program, I would be most grateful if somebody, with a more advanced knowledge, could point out any I might have missed. If there are any edge-cases that have not been accounted for, again, I would be most grateful to you for pointing these out.

  2. I've tried to structure my code for performance, as I have considered size is barely relevant in this instance. I am keen to know if there are further optimizations I can make. Specifically, when it comes to instruction choice, are there any areas where I should substitute an instruction/operand choice for another, in order to improve program efficiency. Or indeed, can you see any particular areas of code where I have been superfluous with my instruction choices. Are there particular lines of code that could have been written differently, more succinctly, to achieve greater efficiency. Indeed, I’ve tried to consider instruction/operand choice carefully, and even changed a large part of my code at one point, in order to avoid the use of the MUL instruction, which I know to be a infamous slug.

  3. I have tried to stick to 32-bit register size (i.e. EAX), even though this is x86-64 code. My understanding is that, in most cases, this is still a more efficient approach than using the 64-bit counterparts, mainly due to the required prefix in the encoding of 64-bit instructions. Likewise, I have also tried to avoid using 8-bit/16-bit operands alongside the MOV instruction, as I understand this would be significantly slower than using the 32-bit counterpart. Hence I’ve stuck with the extended (32-bit) registers, regardless of the operand size, is this correct?

  4. I have used the LEA instruction quite liberally throughout. Is my usage appropriate? I find it a very useful component of the instruction set. My understanding is that it is also fast and compact.

  5. I have tried to structure my code in such a way that unconditional jumps are kept to a minimum, although there are still two which I cannot figure a way out of. Are unconditional jumps particularly problematic in assembly code? I have read somewhere that it is preferable to avoid them where possible. Why is this so?

  6. With regards to Labels, are there hard and fast rules that good (Assembly) programmers live and die by? I have tried to keep them descriptive and as short as possible. Although, one in particular, "ConvertControlChars" seems a little on the long side. Any thoughts?

  7. To handle complexity, I have modularised the code. I have used procedures where I considered there was a distinct section of code, that was clearly a branch to the main program, that achieved a clear and concise task. I also took into consideration whether or not the procedure itself may in fact be useful to add to a library, for reuse on future projects. Macros I have mainly written to handle the register loads for kernel syscall interrupts, to save on key-strokes, and to reduce the visible lines of source code.

  8. Finally, is there any obvious restructuring that would improve the performance of the code?

Ultimately, my main concern is performance. I would like to know where my code can be made more efficient, what I should do differently, and what I should be thinking about going forward.

Notes:

i. In my project, macros are treated as %include files, and procedures are assembled separately into their own object file. 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


Run with the following commands: ./hexdumpadvanced < [Input_File]

SECTION .data                           ; section containing initialised data

    ErrorMSG: db "There has been an unexpected error, your program has terminated",0Ah
    ERRORLEN: equ $-ErrorMSG

    ZeroInput: db "The input file did not contain any data, the program has terminated",0Ah
    ZEROLEN: equ $-ZeroInput

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

; ASCII table. The high 128 characters are translated to ASCII period (2Eh). The non-printable characters in the low 128 are also translated to ASCII period, as is char 127.

    PeriodXLat: db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh   
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 20h, 21h, 22h, 23h, 24h, 25h, 26h, 27h, 28h, 29h, 2Ah, 2Bh, 2Ch, 2Dh, 2Eh, 2Fh
                db 30h, 31h, 32h, 33h, 34h, 35h, 36h, 37h, 38h, 39h, 3Ah, 3Bh, 3Ch, 3Dh, 3Eh, 3Fh
                db 40h, 41h, 42h, 43h, 44h, 45h, 46h, 47h, 48h, 49h, 4Ah, 4Bh, 4Ch, 4Dh, 4Eh, 4Fh
                db 50h, 51h, 52h, 53h, 54h, 55h, 56h, 57h, 58h, 59h, 5Ah, 5Bh, 5Ch, 5Dh, 5Eh, 5Fh
                db 60h, 61h, 62h, 63h, 64h, 65h, 66h, 67h, 68h, 69h, 6Ah, 6Bh, 6Ch, 6Dh, 6Eh, 6Fh
                db 70h, 71h, 72h, 73h, 74h, 75h, 76h, 77h, 78h, 79h, 7Ah, 7Bh, 7Ch, 7Dh, 7Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh
                db 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh, 2Eh

SECTION .bss                                    ; section containing uninitialised data

    InputBuff: resb INPUTLEN                    ; Memory buffer, in which to read data from input file
    INPUTLEN: equ 32                            ; The value of variable 'INPUTLEN' dictates the number of hex-pairs printed per row of terminal output, and also the number of bytes read from file per 'loop'

    Output: resb OUTPUTLEN                      ; Buffer in memory, in which to construct the string(s) used as the main output of the program
    OUTPUTLEN: equ 65536

SECTION .text                                   ; section containing code

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

;-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
; ReadInput   : Invokes x86-64 sys_read. Kernel `syscall` no. 0
; IN          : %1 is the memory offset to read to; %2 is the number of bytes to be read
; Returns     : RAX will contain the number of bytes read to memory
; Modifies    : RAX as the return value; all other registers presevered on stack
; 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. RAX will be clobbered, as it will contain the return value of sys_read:    
    push rcx
    push r11
    push rdi
    push rsi
    push rdx

; Prepare registers, and invoke kernel sys_read:
    mov eax,0                                   ; sys_read
    mov edi,0                                   ; stdin
    mov esi,%1                                  ; Memory offset in which to read input
    mov edx,%2                                  ; Number of bytes to be read from input
    syscall                                     ; Kernel `syscall`

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


;-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
; WriteOutput : Invokes x86-64 sys_write. Kernel `syscall` no. 1  
; 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; all other registers preserved on stack
; 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
    push r11
    push rdi
    push rsi
    push rdx

; Prepare registers, and invoke kernel sys_write:
    mov eax,1                                   ; sys_write
    mov edi,1                                   ; stdout
    mov esi,%1                                  ; Memory offset delimiting the start of data to write to output
    mov edx,%2                                  ; Length (number of bytes) of data to write to output
    syscall                                     ; Invoke kernel `syscall`. 

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


;--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
; ExitProgram : Invokes x86-64 sys_exit. Kernel `syscall` no. 60
; 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
    mov edi,0
    syscall
%endmacro


;-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
; ErrorHandler : Displays error message to output and exits program elegantly
; Updated      : 06/08/2018
; IN           : Nothing
; Returns      : Nothing
; Modifies     : Nothing
; Calls        : Includes 'WriteOutput' and 'ExitProgram', 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

%macro ErrorHandler 0
    cmp rax,0FFFFFFFFFFFFF000h              ; Error range under Linux is -4095 ..... -1
    jna %%exit                              ; If error, i.e. above 0FFFFFFFFFFFFF000h (two's complement of -4096) error has occurred
    WriteOutput ErrorMSG, ERRORLEN          ; Invoke 'WriteOutput macro
    ExitProgram                             ; Invoke 'ExitProgram' macro
%%exit:
%endmacro


;-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
; MoveString  : Moves string from memory offset A to memory offset B; please see Description for more detail
; IN          : %1 is the destination memory offset; %2 is the source memory offset; %3 is the byte count in the string
; Returns     : Nothing
; Modifies    : 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 rsi
    push rcx

    lea edi,%1                                  ; Destination memory address for MOVSB instruction
    lea esi,%2                                  ; Source memory address for MOVSB instruction
    mov ecx,%3                                  ; EAX contains the strings char (byte) count
    rep movsb                                   

    pop rcx
    pop rsi
%endmacro


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

;-------------------------------------------------------------------------------------------------------------------
; CharToHex   : Converts a string of chars in memory, to their underlying binary representations, see Description
; Architecture: x86-64
; IN          : EBP is the memory offset of the string of input chars; EDI is the memory offset of the string of converted
;               converted hexidecimal pairs; EAX 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 
; Calls       : Nothing
; Description : CharsToHex excepts a string of ASCII chars, at offset EBP, and converts the chars to a string of chars
;               representing their underlying binary representations. For example, if char at EBP was "A", then [EBP]
;               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 rbx
    push rcx
    push rbp
    push rax

    mov ecx,eax                         ; Move the count of chars read to memory, to Counter Register ECX
.convertChars:
    mov al,byte [ebp]                   ; Move byte from input buffer to AL
    mov bl,al                           ; Copy char into BL
    and al,0Fh                          ; Bit-mask, AL will now hold lower nibble of hex-pair
    shr bl,4                            ; BL will now hold upper nibble of hex-pair

; Look up each nibble, in turn, in lookup table Digits, and return the underlying binary pattern, ready to write
; to stdout:
    mov al,byte [Digits+eax]            ; Lookup digit in 'Digits' table, return the underlying binary
    mov bl,byte [Digits+ebx]
    mov byte [edi],bl                   ; 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, to store hex-pair plus space char (+3)
    inc ebp                             ; Increment input buffer pointer, to fetch next char from memory
    dec ecx                             ; Decrement the count of chars that are yet to be converted, once zero, we are finished and can exit procedure
    jne .convertChars                   ; If char count not zero, still characters left to convert, jump to '.convertChars'

; Restore registers and return:
    pop rax
    pop rbp        
    pop rcx
    pop rbx
    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
; IN                    : EBP is the offset of the string to be converted; EAX is the number of chars to be converted
; Returns               : Nothing
; Modifies              : Nothing
; Calls                 : Nothing
; Description           : Scans a string of chars in memory. Converts control chars to period (2Eh), printable chars are left unchanged. The procedure uses the x86 instruction XLAT.

ConvertControlChars:
; Preserve registers:
    push rbx
    push rdx
    push rbp                        
    push rax    

; Convert string of EDX length, starting at offset ESI, using XLAT:
    mov edx,eax
.nextChar:
    mov al,byte [ebp]                   ; Fetch char for conversion from memory. By using XLAT, the char value itself, is the index position into the conversion table
    lea ebx, [PeriodXLat]               ; Load offset of translation lookup table
    XLAT                                ; Convert chars

    mov byte [ebp],al                   ; Store converted char back out to memory

    inc ebp                             ; Move pointer to next char in string
    dec edx                             ; Decrement the number of chars left to converted
    jne .nextChar                       ; If zero, i.e. no more chars to be converted, return from call

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


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


GLOBAL _start                                       ; Linker need this to find an entry point

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

; Create pointers for input and output memory buffers. The instructions are situated here in the source code, as the instructions do not need to be repeated each time the program loops: 
        lea ebp,[InputBuff]
        lea edi,[Output] 

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

        add ecx,eax                                 ; ECX will store the aggregate number of bytes read from file

; Check return value from sys_read. If no data has been read, and the program is on first loop (ECX), then there has been no data read from file, inform user and exit program. If there has been data read from
; file (ECX), 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 ConvertChars                            ; If data has been read from file, continue to 'ConvertChars', else fall-through to to test for EOF    

        cmp ecx,0                                   ; Compare loop count to one
        jne PrintOutput                             ; If data has previously been read, then the zero value in EAX indicates EOF, 'PrintOutput' and exit program

        WriteOutput ZeroInput, ZEROLEN          ; Inform user that no data has been read and ask them to try again
        ErrorHandler                                ; Check for error return code from `syscall`, invoked during 'WriteOutput' macro
        jmp Exit                                    ; There has been no data passed to sys_read, user has been informed, exit program 

; Convert each individual char, in 'InputBuff', to a string representing its underlying binary notation, and store at memory offset 'Output'. For example, if char in memory is "A", the underlying
; binary notation will be 41h. Therefore, 'ConvertChars' will create the string 3431h, in memory buffer 'Output'. 3431h printed to stdout will be converted to string "41", the binary notation of char "A":
ConvertChars:
        call CharToHex

; If the number of bytes read from file < 16, add additional space chars (20h) as 'padding' to align output. This is important to align the last row of ouput in the terminal, or if total input is less than 16-bytes:
        cmp eax,INPUTLEN                            ; Compare INPUTLEN to number of chars read from file 
        je CharsToOutput                            ; If INPUTLEN is equal to chars read from file, no padding is required

        mov esi,INPUTLEN                            ; Move maximum number of bytes read from file (INPUTLEN) to ESI
        sub esi,eax                                 ; Subtract the actual number of bytes read (EAX) from ESI, to calculate the size of the necessary padding
        mov edx,esi                                 ; Store the size of the required padding in EDX. The data will be used with 'CharPadding' Label; ESI is decremented during 'RowBuffer' Label
        add ecx,edx                                 ; Add padding byte count to the aggregate count of bytes to write to output

RowBuffer:
        mov dword [edi],00202020h                   ; Add 'padding' to relevant location in 'Output' string
        dec esi                                     ; Decrement count of 'padding' elements
        lea edi,[edi+3]                             ; Calculate memory offset of next padding element, each padding element is 3-bytes in length
        jnz RowBuffer                               ; If more padding required, repeat 'RowBuffer'

; Add vertical bar symbol (7CH) to relevant position in output string.
CharsToOutput:     
        mov byte [edi],7Ch                          
        inc edi                                     ; Increment 'Output' pointer (EDI) to correct memory offset for storing string of ASCII chars

; Convert any non-printable chars in 'InputBuff' to period (2Eh), ready for writing to standard output. The procedure called 'ConvertControlChars' contains a converted ASCII table:  
        call ConvertControlChars                    ; Call XLAT procedure

; Move string of chars from input, to relevant place in output string. A row of chars will appear immediately after the row of related hex-pairs in output, 'book-marked' either end by a vertical bar:
        MoveString [edi], [InputBuff], eax

; Add necessary padding to ASCII char output display column:
        cmp edx,0                                   ; If the difference between INPUTLEN and the number of bytes read from memory (EDX) is zero, no padding required
        je CompleteLine                             ; If no padding required, continue to 'CompleteLine'

CharPadding:
        mov byte [edi],20h                          ; Add space chars (20h) as padding, to align the ASCII char output display colum in the terminal
        inc edi                                     ; Increment 'Output' string memory pointer
        dec edx                                     ; Decrement the required padding count
        jnz CharPadding                             ; If more padding required, repeat process

; Add vertical bar symbol (7CH) to end of row in output display, plus a LF (0Ah). Prepare 'Output' string pointer (EDI) to store next row of output bytes:
CompleteLine:
        mov word [edi],0A7Ch                        ; Add vertical bar (7Ch) to the end of the output row in the terminal. LF (0Ah) to next line in terminal.
        add edi,2                                   ; Increment 'Output' string pointer to account for appending 0A7Ch to the string

; For each line printed to the terminal there will be 2 x vertical bars (7Ch), plus a LF (0Ah), added to the total number of bytes printed to stdout. Record the running total, for use in 'PrintOutput' calculation:
        add ecx,3

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

; Write 'Output' to terminal. For each loop through 'ReadFile' (ECX), there will be INPUTLEN x 4 bytes printed to the terminal [each input char is converted to a hex-pair + a space char + the char itself].
; The aggregate count of vertical line chars (7Ch) and LF chars (0Ah), has also been added to the total byte count:   
PrintOutput:
        lea edx,[ecx*4]                             ; 
        WriteOutput Output, edx                     ; Write output, using macro 'WriteOutput'
        ErrorHandler                                ; Handle errors

; Exit program elegantly:
Exit:
        ExitProgram                                 ; 'ExitProgram' macro, will exit the program elegantly. From library "system_call_macros"

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

I see a number of things that may help you improve your program.

Keep writing good comments

The comments for the macro and procedure headers are quite good. They convey, in a systematic way, just what the reader needs to know. My custom is also to add a "Trashed" section that says which registers are used and not restored. More on that later. One area that could be improved, however, would be to describe how registers are used in the main program. Also, make sure the comments don't mislead -- the comments for the ErrorHandler macro, for instance, are incorrect.

Consider using shorter lines

Both the source code and the output of the code are very long -- over 200 characters. If one has a nice wide display that's not necessarily a problem, but not every programmer is so equipped. Many years ago, the standard was that each line should be no more than 80 characters wide because that was what fit on a printed sheet of paper. You may elect to choose a wider line, but consider that other programmers may literally be unable to read your code if you do.

Measure your program

Programming in assembly language is fun and interesting for many. That in itself is sufficient reason to write programs using it, but a program like this spends almost all of its time reading and writing data, so even if all of the processing took no time at all, the performance is going to be limited by the speed of I/O. To check this, it's useful to measure the code, and one good way to do that is by using a profiler. Two that are commonly used under Linux are gprof and oprofile.

Fix the bug

I tried the program on a 1.5Gib file and was rewarded with a system crash. Ultimately, this is caused because of the relatively small fixed output buffer size and the way the program handles data. At the moment, it reads the entire input file and attempts to create (in 64K) the entire output. That's guaranteed to fail with even modestly sized inputs and should be fixed. One way to do so would be to handle data line at a time.

Avoid slow instructions

The xlat instruction, as you've used it, seems to make sense because it does what you intend. That is, it translates from one encoding to another using a table. However, it's a relatively slow instruction and definitely not needed here. What could be done instead would be to check if the value of the byte is in the range 0x20 .. 0x7E. If it is, print the value; otherwise print '.'. This also has the consequence of eliminating the PeriodXLat table and easily shrinking the program by 256 bytes.

Use fast instructions

The padding code could be written a bit differently to use rep stosb. This instruction has some optimizations built into it on some processors, making it faster than the existing code.

Use small 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, edi  ; so does this but with a 3-byte instruction

The difference, however, is that the latter instruction is two bytes shorter. It also sets the flags register (unlike a mov) so don't use it if you need to preserve flags.

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 generally 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.

Use memory efficiently

Right now, there is an input area and an separate output area. If, as mentioned above, you modify the program to handle input line at a time, one can eliminate the input buffer entirely by simply reading the data into place in the output buffer and then using something like ConvertControlChars in-place on that data.

Avoid extra processing

If each output line is of the same format, we already know where the vertical bars and spaces and linefeed will be. For that reason, I'd suggest setting all of that up in memory once and then reusing the buffer, only overwriting the changeable parts.

Define constants before use

The code currently has these lines:

InputBuff: resb INPUTLEN
INPUTLEN: equ 32

However, when assembling, NASM says warning: forward reference in RESx can have unpredictable results. What it means is, essentially, that you need to reverse those two lines so that the definition for INPUTLEN comes before it is used:

INPUTLEN: equ 32
InputBuff: resb INPUTLEN

Even if NASM had no opinion on this, it's also nicer for a reader of the code.

Consider adding an offset

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

Be wary of using rbp as a data pointer

While the use of rbp or ebp works as a data pointer in this case, it's worth remembering that the system architecture uses a segment register as well. For most registers, such as edi and esi, that's ds, but for ebp it's ss unless specifically overridden. It works here because Linux sets up the segment registers to alias to the same location, but it's not generally true in the architecture. For this reason, it's good to at least be aware of the potential issue, and perhaps best to avoid it by not using the ebp register in that way if other registers are available. Its usual use is as the stack frame pointer.

Rethink error handling

Ordinarily, I'm a staunch advocate for thorough error checking and handling, but this code might be something of an exception (if you'll pardon the pun). For example, if the write to stdout fails, the current error handler attempts to ... write to stdout again with an error message. I'd suggest either trying to write to stderr instead or maybe even just silently bailing out if there's an error.

\$\endgroup\$
  • \$\begingroup\$ “Be wary of using RBP as a data pointer”, this is exactly the type of gem of knowledge I was looking for, thank you. The majority of texts I have read so far, really just group all of these registers together as “general purpose” registers. This gives the impression that they are all the same and can be used interchangeably. I suspect this is not entirely the case, and that the different registers have subtle differences, at least historically. \$\endgroup\$ – Andrew Hardiman Aug 16 '18 at 13:16
  • 1
    \$\begingroup\$ I added a few other notes to my answer which you may find useful. \$\endgroup\$ – Edward Aug 16 '18 at 15:07
  • \$\begingroup\$ Great stuff, thank you. Can I ask, you’ve mentioned that “both the source code and the output of the code are very long”. Are you referring mainly to the comments within the source code? As, the output in the terminal does not exceed 66 characters (including spaces). I think my main concern was that, by wrapping the text, at say a column count of 80, certain comments sections would become excessively bulky. \$\endgroup\$ – Andrew Hardiman Aug 16 '18 at 15:35
  • \$\begingroup\$ By my count, each line the output (with the given value of 32 for INPUTLEN) is 130 characters which is on the long side. For the source code, yes, it's the comments that make the lines so long. Detailed bulk comments can start at the first column and be broken into multiple lines. I find such code easier to work with. See beroset.com for a number of examples of assembly language code. \$\endgroup\$ – Edward Aug 16 '18 at 15:47
  • \$\begingroup\$ My apologies, you are correct. I appreciate the links to further resources/learning materials. In particular, the heads-up with regards to gprof and oprofile. These are exactly the type of tools I need. I have been chopping and changing instructions a fair bit, based on what I have read about performance etc. However, I see that trying to guess performance improvements based on the choice of single instructions is, well, far too simplistic to say the least. \$\endgroup\$ – Andrew Hardiman Aug 16 '18 at 15:55
4
\$\begingroup\$

I'll just add that CharToHex assumes that the high bytes of EAX and EBX are zero. In particular, the AH value will be non-zero for an input string length greater than 255 characters. When you look up the digit with [Digits+eax] you can access out of bounds memory.

\$\endgroup\$
  • \$\begingroup\$ Thank you. What would be your solution? My thinking is to use xor eax,eax prior to loading the bytes from memory. Would this be the best solution in your experience? Many Thanks. \$\endgroup\$ – Andrew Hardiman Aug 18 '18 at 14:30
  • 1
    \$\begingroup\$ @case_2501 Since you only change the low byte of those registers within the loop, you can put xor eax,eax and xor ebx,ebx before the .convertChars label to zero out the high bytes before the loop. \$\endgroup\$ – 1201ProgramAlarm Aug 18 '18 at 21:36

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.