Optimizer

[John Z]

I just ran across one of my programs that compiles and runs fine under v11 with max speed optimizations and extras checked but compiling the exact same sources under v12 introduces a hidden (for lack of a better term) error where a procedure using GetOpenFileNameW always fails.   If I #pragma optimize(  none  ) just for the specific procedure then it works like V11. So far I've been unable to replicate in a small example....so there must be optimization interactions with other parts of the source code even though un-optimizing  just this procedure 'fixes' it.  Whatever 'it' is is not limited to speed or extras - every optimize setting introduces the problem...

It would be nice to have both more granularity and more (some?) verbosity in what the optimizer changes.  Maybe unreasonable but shouldn't hurt to ask 

John Z

[frankie]

John try to post the assembler generated in the procedure with and without the optimization, this could help to identify the problem.

[John Z]

Good suggestion frankie.

I extracted the 'offending' procedure from the big source module into its own source module.
Stripped almost everything down to the bare minimum 1168 bytes non-optimized object file and
a 1110 bytes optimized object file - much better than the two ~97kb files  and the issue is still
exists, thankfully.  In addition the ~58 byte difference is consistent with the difference in the 97kb files,
so the issue is captured.
Attached are the two object files and the basic streamlined & stripped down C procedure to the minimum.

Just thinking maybe another approach would be use V11 with optimization on the same code to compare with V12 optimization....

When the code fails the GetOpenFileNameW(&ofn) function immediately returns false and never even tries to display the open dialogue.  Maybe I should add GetLastError() and see if that clarifies anything...

Appreciate any insights you have,
Thanks
John Z

Update:  GetLastError says everything is fine even though it failed to display the dialogue . . . oh well 

[John Z]

Well the more I test it the more I think it is not an optimizer issue.
The optimization in v12 is probably exacerbating a hidden memory overflow
issue somewhere in my code that v11 didn't impact.

Anyway I hope you didn't spend too much time on this.

Hopefully I can find the root cause elsewhere . . . .

Thanks,
John Z

[frankie]

Well the more I test it the more I think it is not an optimizer issue.
The optimization in v12 is probably exacerbating a hidden memory overflow
issue somewhere in my code that v11 didn't impact.

I think the same. Looking at the generated code, see image below, the 2 versions are the same apart from the position of the ofn structure and the filename buffer that are exchanged in memory. Then one version calls the memset function while the other implements the last locally using string instructions.
Last difference is the check of return value of function GetOpenFileNameW, made with a 'cmp eax,0' in the unoptimized case, and with a 'test eax,eax' in the other case (that is a shorter and much more efficient coding).
Better you check the whole code, initializations, allocations (static, auto, dynamic), ecc. 

[John Z]

Thanks for the help frankie!

Some more evidence for a memory issue:
I removed all optimizations then added by #pragma only optimizing the one procedure.
The 'bug' was there as expected.

You might recall from above that un-optimized and optimized file2 objs were different by about 58 bytes.
so I changed
wchar_t szFileName[MAX_PATH];
into
wchar_t szFileName[MAX_PATH+57];

The problem went away!  Less than +57 it is back .....
interestingly adding in a new variable k[57] though didn't fix it.

These memory issues are very hard to find....no really good free tools that I know of either,
just tedious detailed examinations ....

Say - what disassembler did you use?

Thanks again,

John Z

[frankie]

John I don't think this issue is related to optimization problem or other memory issue.
Looking at source there is no apparent error.
MS docs states that the function can return false if:

<link>...
If the buffer is too small, the function returns FALSE and the CommDlgExtendedError function returns FNERR_BUFFERTOOSMALL. In this case, the first two bytes of the lpstrFile buffer contain the required size, in bytes or characters.

This behavior is consistent with the widening of the buffer.
Another problem is related to the buffer that should be initialized as empty, but you don't do it. From MS documentation above:

The file name used to initialize the File Name edit control. The first character of this buffer must be NULL if initialization is not necessary. When the GetOpenFileName or GetSaveFileName function returns successfully, this buffer contains the drive designator, path, file name, and extension of the selected file.

You can set it as follows:

Code: [Select]

wchar_t szFileName[MAX_PATH] = {0};

Thanks for the help frankie!
Say - what disassembler did you use?
John Z

I used pedump.exe from PellesC suite 

[John Z]

Thanks frankie!

I'm going to give the CommDlgExtendedError function a try.  I tried the easy GetLastError() which probably was not enough.

I'll zero out the space as you suggest and the doc shows I should, I missed that.  The only caveat is that I use essentially the exact same procedure sequence in 6 other places and also with GetSaveFileNameW.  Optimizing those has not shown any issues.  Nevertheless always best to do it 'right' 

Thanks for the podump tip! I guess I need to investigate the Pelles BIN directory a bit more ....

Back to the code .....

Appreciate all the help very much! 

John Z

[John Z]

Another problem is related to the buffer that should be initialized as empty, but you don't do it. From MS documentation above:

The file name used to initialize the File Name edit control. The first character of this buffer must be NULL if initialization is not necessary. When the GetOpenFileName or GetSaveFileName function returns successfully, this buffer contains the drive designator, path, file name, and extension of the selected file.

You can set it as follows:

Code: [Select]

wchar_t szFileName[MAX_PATH] = {0};

frankie - THANK YOU SO MUCH!  I wish I could buy you a beer or a KEG!

I first used DWORD err= CommDlgExtendedError(); it showed
FNERR_INVALIDFILENAME
0x3002  A file name is invalid.

Then as you said I set wchar_t szFileName[MAX_PATH] = {0};
Problem solved!!! Works perfectly with optimization.  I'll fix the other incidences too.

I really, really, appreciate you sharing your expertise and knowledge!  You saved me innumerable hours and grief.

Thanks very much!

John Z

Update:  All the other incidences worked because I immediately set the filename to  szFileName before it was used.  So all consistent.

[frankie]

You're welcome.
Cheers!!! 

  .   *   ..  . *  *
*  * @()Ooc()*   o  .
    (Q@*0CG*O()  ___
   |\_________/|/ _ \
   |  |  |  |  | / | |
   |  |  |  |  | | | |
   |  |  |  |  | | | |
   |  |  |  |  | | | |
   |  |  |  |  | | | |
   |  |  |  |  | \_| |
   |  |  |  |  |\___/
   |\_|__|__|_/|
    \_________/


P.S. The unptimized code probably worked because the filename storage was in a stack address containing zeroes at beginning (NULL string).
Also this should work:

Code: [Select]

wchar_t szFileName[MAX_PATH];
szFileName[0] = '\0';


[HellOfMice]

Msdn says:

lpstrFile

Type: LPTSTR

The file name used to initialize the File Name edit control. The first character of this buffer must be NULL if initialization is not necessary. When the GetOpenFileName or GetSaveFileName function returns successfully, this buffer contains the drive designator, path, file name, and extension of the selected file.
If the OFN_ALLOWMULTISELECT flag is set and the user selects multiple files, the buffer contains the current directory followed by the file names of the selected files. For Explorer-style dialog boxes, the directory and file name strings are NULL separated, with an extra NULL character after the last file name. For old-style dialog boxes, the strings are space separated and the function uses short file names for file names with spaces. You can use the FindFirstFile function to convert between long and short file names. If the user selects only one file, the lpstrFile string does not have a separator between the path and file name.
If the buffer is too small, the function returns FALSE and the CommDlgExtendedError function returns FNERR_BUFFERTOOSMALL. In this case, the first two bytes of the lpstrFile buffer contain the required size, in bytes or characters.

https://www.flickr.com/photos/studiodxavier/53247034656/in/pool-nikon_strobist/

[John Z]

Thanks HellOfMice!


This is also what frankie schooled me on.  My oversight on setting null to the buffer when I did not needed to initialize it to a specific filename.


John Z