Rookie mistake: spot the error

[dedndave]

ok - i wasn't counting the code i didn't write   :P

[NoCforMe]

Guys, guys, guys: You're completely missing the point. You can't see the forest for the trees.

The code I posted doesn't work correctly. There's a glaring error in it (hence the "rookie mistake"). I finally fixed it, and I posted this as a possible help to others.

It wasn't intended as an exercise on how to generate the absolute smallest code.

I gave you a problem with changing a tire on a car and you all responded with stuff on quantum mechanics. Sheesh.

So class, can anyone tell me what the problem was with my code? (BTW, the structure is a LOCAL, but it doesn't make any difference.)

[oex]

It wasn't intended as an exercise on how to generate the absolute smallest code.

So class, can anyone tell me what the problem was with my code?

It wasn't small enough?

[drizz]

jj, I didn't realize it was a pissing competition.   :red

[dedndave]

Guys, guys, guys: You're completely missing the point. You can't see the forest for the trees.

The code I posted doesn't work correctly. There's a glaring error in it (hence the "rookie mistake"). I finally fixed it, and I posted this as a possible help to others.

It wasn't intended as an exercise on how to generate the absolute smallest code.

I gave you a problem with changing a tire on a car and you all responded with stuff on quantum mechanics. Sheesh.

So class, can anyone tell me what the problem was with my code? (BTW, the structure is a LOCAL, but it doesn't make any difference.)

:lol

Code Select Expand

; First, zero out entire structure:
PUSH EDI
LEA EDI, ofn
MOV ECX, SIZEOF OPENFILENAME
MOV ofn.lStructSize, ECX ;Stuff length while we have it.
XOR EAX, EAX
REP STOSB ;Zero it out.
POP EDI
the problem that i see is that you set the lStructSize member, then overwrite it with 0(s)
that probably generates an ERROR_INVALID_PARAMETER or ERROR_INVALID_DATA error

[NoCforMe]

Not even close, and no cigar.

Wow, maybe it wasn't that glaring an error after all. As I said, I was banging my head against the keyboard for a good half hour before I spotted it.

Hint: the error I got back from GetOpenFileName() was CDERR_STRUCTSIZE. (That's too big a hint, actually. And you have to call CommDlgExtendedError() to retrieve the error code.)

@Dave: Bingo! You got it. (Actual error is shown above.)

[jj2007]

jj, I didn't realize it was a pissing competition.   :red

of course, we shouldn't do this in the campus, but it's fun   :bg

OK, maybe you are right that we shouldn't play such games in the Campus. The Forum rules clearly say it's a humour-free zone :red

Therefore, for all those who want to learn assembly in the Campus (OP code):

Code Select Expand

MOV ofn.lStructSize, ECX ;Stuff length while we have it. <<< that's ok

XOR EAX, EAX
LEA EDI, ofn      ; that points to the same memory as ofn.lStructSize
REP STOSB      ; and here we mercilessly overwrite the ecx value



Still, those who want to write correct and elegant code should study Dave's example. Even in the Campus :bg

[dedndave]

the problem that i see is that you set the lStructSize member, then overwrite it with 0(s)

i thought that's what i said   :P

[NoCforMe]

Yes, the co-winners are jj and Dave. You may collect your prize (a brand-new shrink-wrapped copy of DOS 3.1) as you leave.

By the way, I vote in favor of fun, at all times. We can do things the correct way, or we can do things the fun way ...

[jj2007]

Guys, guys, guys: You're completely missing the point. You can't see the forest for the trees.

The code I posted doesn't work correctly.

We surely saw the forest. All examples posted work correctly, and yes everybody saw the "glaring mistake" but nobody in here smokes cigars  :wink

[qWord]

... and as we all know, the direction flag is a constant :wink

[dedndave]

seeing as we are on the subject of the OPENFILENAME structure size   :bg

if you look on MSDN, it is defined as

Code Select Expand

typedef struct tagOFN {
  DWORD         lStructSize;
  HWND          hwndOwner;
  HINSTANCE     hInstance;
  LPCTSTR       lpstrFilter;
  LPTSTR        lpstrCustomFilter;
  DWORD         nMaxCustFilter;
  DWORD         nFilterIndex;
  LPTSTR        lpstrFile;
  DWORD         nMaxFile;
  LPTSTR        lpstrFileTitle;
  DWORD         nMaxFileTitle;
  LPCTSTR       lpstrInitialDir;
  LPCTSTR       lpstrTitle;
  DWORD         Flags;
  WORD          nFileOffset;
  WORD          nFileExtension;
  LPCTSTR       lpstrDefExt;
  LPARAM        lCustData;
  LPOFNHOOKPROC lpfnHook;
  LPCTSTR       lpTemplateName;
#if (_WIN32_WINNT >= 0x0500)
  void          *pvReserved;
  DWORD         dwReserved;
  DWORD         FlagsEx;
#endif
} OPENFILENAME, *LPOPENFILENAME;

the new members apply to everything windows 2000 and above
however, the windows.inc file defines the structure without them

if you refer to this post, you will see that i hard-code the size value so that it assembles the same if windows.inc is updated
http://www.masm32.com/board/index.php?topic=17525.msg147288#msg147288

[NoCforMe]

... and as we all know, the direction flag is a constant :wink

Good point, and I thought of that as I was coding this: what if someone leaves the direction flag set?

Made me think that, while I've never seen the innards of Windows code (I mean the OS), I imagine every place that uses string moves/stores/scans probably does something like this:

Code Select Expand


pushf
cld

(operation involving STOSx, SCASx, MOVSx, CMPSx}

popf


in order to "bulletproof" it, don't you think?

[jj2007]

By default, the direction flag is cleared. If you have to set it (rare case), it's your duty to clear it on exit. You would use cld, not a slow pushfd/popfd pair.

[dedndave]

it's part of the ABI definition - that the DF should remain cleared
in fact, some API functions will hang or have other problems if called with it set - some get really slow

as a side note, instructions that set or clear this flag (POPFD, CLD, STD) execute surprisingly slow

if i am interested in speed, i will sometimes test the flag bit, executing CLD only if it is set