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