push 0

[Nyatta]

You should test the number of bytes read

Code Select Expand


cmp DWord Ptr [ecx],0
jne EOF

cmp DWord Ptr [ecx],1024
jne Error


The code is not very clear, comment it and rewrite it.
Mine is not always very easy to understand but here you make worst than I.

It is intended to load any file size, but print a max of 1 KB of data, I don't understand the need to test the filesize for any means other than informing the user the file read was empty, as that would crash the program.
I added comments more as step-by-step means to keep track of what data's where, as this was started on my 3rd day using MASM or any assembler. :icon_confused:

Code Select Expand

POP EBX ;Move BYTEs read from stack to EBX.
print str$( EBX ), " BYTEs read.", 13, 10

CMP EBX, 0 ;If zero BYTEs are read
JE closeProg ;jump to closing sequence.

this is not very sound practice
write the code so that it will work, pass or fail - and exit at the same point
notice that EBX, ESI, EDI, and EBP are preserved across API calls
you can use those resisters to hold values without PUSH/POP

Code Select Expand

.IF !EAX ;Check for error,
POP EBX ;if true move BYTEs read out of the way
POP EBX ;retrieve handle,
INVOKE CloseHandle, EBX ;close handle,
inkey LastError$(), 13, 10 ;display the error
exit ;and close the program.
.ENDIF

What portion of this as practice isn't sound? Printing more data than the console holds within it's history?
My logic for split exit points was to avoid the additional JUMP where I felt it was unneeded (saving 3 clocks); I was under the impression that having a single exit point boiled down to personal preference, though it can result in lost readability. I'm trying my best to lean towards speed over readability where rational, in part to hardwire speed-based systems into my learning, but also to expand my comprehension. (I'd use a loop instead of repeating the same macro an irrational number of times.) I'm not trying to make it seem like anyone here is the bad-guy, I'm just explaining what I, most likely incorrectly, see here.
Thank you for pointing out the preservation of those registers, I will edit my code to incorporate those.

[dedndave]

conditional pushing and popping is asking for trouble (although, i have done it myself - lol)

and - having a common exit point makes more sense
try writing the code so there is only one close handle call
then, branch as required - perhaps to the exit point

[dedndave]

also - handles and counts, etc - don't need to always be kept in registers with push/pop
it sometimes makes for cleaner code to store the value in a memory variable
that way, push and pop mismatches are less likely to cause problems later on

[dedndave]

you may see some of us do the push/pop thing a lot
we are very comfortable with use of the stack 

it's not necessarily the best code, though

[Nyatta]

conditional pushing and popping is asking for trouble (although, i have done it myself - lol)

The pop occurs and the program exits, but I understand you mean that in the broad scope of programming. Those pops were removed when I changed the "PUSH EAX" before "PUSH 0" to "MOV EDI, EAX", it cut down on few lines all around. :t

Code Select Expand

.IF !EAX ;Check for error,
INVOKE CloseHandle, EDI ;close handle,
inkey LastError$(), 13, 10 ;display the error
exit ;and close the program.
.ENDIF

and - having a common exit point makes more sense
try writing the code so there is only one close handle call
then, branch as required - perhaps to the exit point

My code makes it impossible for CloseHandle to be called twice, and as I said, I feel I am increasing the clock count by 3 by branching when a condition that triggers closing procedures are met. It just sounds more like a decision between file-size and clocks... and I was practicing optimisation tactics here.
I'm uncertain on why I'm being told to branch beyond that, that's what I'm asking for information on.

also - handles and counts, etc - don't need to always be kept in registers with push/pop
it sometimes makes for cleaner code to store the value in a memory variable

I used a PUSH 0 and directed lpNumberOfBytesRead from the ReadFile function to this address because it requests a pointer to a variable, and not a register. If it were faster for the computer to read from the memory I would do that, until then I aim to stay away until I hit a dead-end and need to store some data.

[Grincheux]

After exit you don't POP EDI!

[Nyatta]

After exit you don't POP EDI!

Doesn't the exit macro trigger clean-up of the process space before turning it over to the OS?

Code Select Expand

    exit MACRO optional_return_value
      IFNDEF optional_return_value
        invoke ExitProcess, 0
      ELSE
        invoke ExitProcess,optional_return_value
      ENDIF
    ENDM
The MSDN page on ExitProcess states:
"6. All of the object handles opened by the process are closed."
Does the stack have a handle that once cleared leaves the remanence within the RAM until it's either over-written or degrades?

[qWord]

After exit you don't POP EDI!

Doesn't the exit macro trigger clean-up of the process space before turning it over to the OS?

Yes. Also, ExitProcess does never return, thus following code would be dead code.

BTW, you could make your live much easier -- without any performance lost -- by using local variables rather than this error-prone,  unmaintainable and unreadably push/pop technique.

[Nyatta]

Yes. Also, ExitProcess does never return, thus following code would be dead code.

Exactly my intended outcome if the condition is met, but from the looks of what I'm being told I used a fairly unconventional method.

BTW, you could make your live much easier -- without any performance lost -- by using local variables rather than this error-prone,  unmaintainable and unreadably push/pop technique.

No performance loss in mind I will certainly take a look as it sounds it will make the whole process far easier. Ollydbg is simplifying all of this by far have I make a mistake in the stack, it's getting far easier to track registers and the stack.
Isn't my buffer variable, "fBuffer", a local variable as it's data is written to as the program runs?

[qWord]

Local variables are local to a function or procedure and are setup on the stack when entering a function and destroyed when returning from it. MASM does that for you automatically when using PROCs with the keyword LOCAL (use OllyDbg to see how it is done exactly):

Code Select Expand

include \masm32\include\masm32rt.inc

.code
main proc
LOCAL hFile:HANDLE, buffer[123]:CHAR, numberOfBytesRead:DWORD

  mov hFile, fopen("...")
  ; ...
  invoke ReadFile, hFile, ADDR buffer, SIZEOF buffer, ADDR numberOfBytesRead, 0
  ; ...
 
  invoke CloseHandle, hFile
 
  exit
 
main endp
end main
Remarks that the total size of local variables in a PROC should not exceed 4096 Bytes (= size of 1 page), because there is a guard-page (access to that page cause an exception) below the stack that must be touched to increase the stack size (the system does that in the corresponding exception handler). There is also the possibility to allocate more than 4KB, but this requires to "probe the stack" ...  but that is another story.

[jj2007]

the total size of local variables in a PROC should not exceed 4096 Bytes (= size of 1 page), because there is a guard-page (access to that page cause an exception) below the stack that must be touched to increase the stack size (the system does that in the corresponding exception handler).

You can see the effect in the debugger when playing with the size of a local buffer:

Code Select Expand

include \masm32\include\masm32rt.inc

.code
MyTest proc arg
LOCAL LocBuffer[3*4096+3960]:BYTE ; try +3970
  lea eax, LocBuffer
  int 3 ; for Olly
  dec dword ptr [eax]    ; this modifies the start of the buffer
  ret
MyTest endp

start: invoke MyTest, 123
print "ok"
exit
end start

[dedndave]

if i  were to write such a program, it might look something like this...

Code Select Expand

        INCLUDE     \masm32\include\masm32rt.inc
        .686

;#################################################################################

FileSize  PROTO :LPSTR
OpnFile   PROTO :DWORD,:LPSTR

;#################################################################################

RawFileType     EQU 16
FileBufferSize  EQU 1024

;*********************************************************************************

OPF_FILECREATE  EQU GENERIC_WRITE or CREATE_ALWAYS                  ;(40000002h)
OPF_FILEWRITE   EQU GENERIC_WRITE or OPEN_EXISTING                  ;(40000003h)
OPF_FILEREAD    EQU GENERIC_READ or OPEN_EXISTING                   ;(80000003h)
OPF_FILERANDOM  EQU GENERIC_READ or GENERIC_WRITE or OPEN_EXISTING  ;(C0000003h)

;#################################################################################

        .DATA
        ALIGN   4

szFileName      db "Audio_Seg.raw",0

szFileErrMsg    db "Error Reading Input File"
szExitMsg       db 13,10,"Press any key to exit ...",13,10,0

;*********************************************************************************

        .DATA?
        ALIGN   4

hFileHandle     HANDLE ?
dwBytesRead     dd ?

FileBuffer      db FileBufferSize dup(?)

;#################################################################################

        .CODE

;*********************************************************************************

main    PROC

    INVOKE  FileSize,offset szFileName
    .if ecx==INVALID_FILE_ATTRIBUTES
        mov     edx,offset szFileErrMsg
    .else
        mov     ebx,FileBufferSize
        .if !(edx) && (eax<ebx)
            xchg    eax,ebx             ;EBX = lesser of file size or buffer size
        .endif
        INVOKE  OpnFile,OPF_FILEREAD,offset szFileName
        .if eax==INVALID_HANDLE_VALUE
            mov     edx,offset szFileErrMsg
        .else
            mov     hFileHandle,eax
            INVOKE  ReadFile,eax,offset FileBuffer,ebx,offset dwBytesRead,NULL
            push    eax
            INVOKE  CloseHandle,hFileHandle
            pop     eax
            .if eax

                ;add your code here
                ;to process data from the buffer

                mov     edx,offset szExitMsg
            .else
                mov     edx,offset szFileErrMsg
            .endif
        .endif
    .endif
    inkey   edx
    exit

main    ENDP

;*********************************************************************************

FileSize PROC lpszFileName:LPSTR

;File Size Function

;Call With: lpszFileName = address of zero-terminated filename string
;
;  Returns: EDX:EAX = file size
;           ECX     = file attributes, INVALID_FILE_ATTRIBUTES if file not found (-1)
;
;Also Uses: all other registers are preserved

;-----------------------------------------

    LOCAL   _w32fds :WIN32_FILE_ATTRIBUTE_DATA

;WIN32_FILE_ATTRIBUTE_DATA STRUCT
;  dwFileAttributes DWORD ?
;  ftCreationTime   FILETIME <>
;  ftLastAccessTime FILETIME <>
;  ftLastWriteTime  FILETIME <>
;  nFileSizeHigh    DWORD ?
;  nFileSizeLow     DWORD ?

;-----------------------------------------

    xor     ecx,ecx
    mov     _w32fds.dwFileAttributes,INVALID_FILE_ATTRIBUTES
    mov     _w32fds.nFileSizeHigh,ecx
    mov     _w32fds.nFileSizeLow,ecx
    INVOKE  GetFileAttributesEx,lpszFileName,ecx,addr _w32fds
    mov     edx,_w32fds.nFileSizeHigh
    mov     eax,_w32fds.nFileSizeLow
    mov     ecx,_w32fds.dwFileAttributes
    ret

FileSize ENDP

;*********************************************************************************

        OPTION  PROLOGUE:None
        OPTION  EPILOGUE:None

OpnFile PROC    dwOpenFlags:DWORD,lpszFileName:LPSTR

;File Open Function

;Call With: lpszFileName = address of zero-terminated filename string
;           dwOpenFlags  = combined creation and access flags:
;                          bits 0 and 1   = dwCreationDisposition flag(s)
;                              typically, CREATE_ALWAYS to create a new file,
;                              or OPEN_EXISTING to open an existing file
;                          bits 30 and 31 = dwDesiredAccess flag(s)
;                              typically, GENERIC_WRITE to write to a file,
;                              or GENERIC_READ to read a file,
;                              or both flags for read/write ("random") access
;
;  Returns: EAX = file handle, INVALID_HANDLE_VALUE if unable to open file (-1)
;
;Also Uses: ECX and EDX are destroyed, all other registers are preserved

;-----------------------------------------

    mov     eax,[esp+4]
    xor     ecx,ecx
    movzx   edx,al
    and     eax,0C0000000h
    and     dl,3
    INVOKE  CreateFile,[esp+32],eax,FILE_SHARE_READ,ecx,edx,FILE_ATTRIBUTE_NORMAL,ecx
    ret     8

OpnFile ENDP

        OPTION  PROLOGUE:PrologueDef
        OPTION  EPILOGUE:EpilogueDef

;#################################################################################

        END     main

[Grincheux]

Easy to read.
Just verify the number of bytes read.

[dedndave]

easy enough
just change this line

Code Select Expand

            INVOKE  CloseHandle,hFileHandle
            pop     eax
            .if eax

to this

Code Select Expand

            INVOKE  CloseHandle,hFileHandle
            pop     eax
            .if (eax) && (ebx==dwBytesRead)

[Grincheux]

That was a joke

Code Select Expand



test eax,eax
jz Eof

sub eax,FileBufferSize
jnz Error_1


and

Code Select Expand

mov     edx,_w32fds.nFileSizeHigh

It is ignored and the buffer will not be big enougth : 4 294 967 296 bytes if edx = 1 and eax = 0h