Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stacktrace doesn't work with non-ASCII or Unicode identifiers #15138

Closed
wxtrac opened this issue Apr 6, 2013 · 36 comments
Closed

Stacktrace doesn't work with non-ASCII or Unicode identifiers #15138

wxtrac opened this issue Apr 6, 2013 · 36 comments

Comments

@wxtrac
Copy link
Collaborator

@wxtrac wxtrac commented Apr 6, 2013

Issue migrated from trac ticket # 15138

component: wxMSW | priority: normal | resolution: fixed | keywords: Unicode call stack trace non-ASCII work-needed

2013-04-06 13:35:36: suzumizaki (Suzumizaki-Kimitaka) created the issue


Current stack-tracing code always fails if some functions have non-ASCII identifiers with wxMSW.

When the assertion-stop is made while calling the function with non-ASCII named, nonsense another stop will also be made. "Nonsense" one throws away the information we really need, which the first one has.

This invalid behavior depends on calling wxString::FromAscii(char*) with non-ASCII string in short. But with accuracy to say, we should use wide char (PCWSTR/PWSTR) versions of debugging API on Windows, not ANSI(MBCS) ones.

As you know, C++ specification allows non-ASCII identifiers. This means the names can be exists outside of wxWidgets and the outside one can be included into our stack traces.

I tried to fix and post the patch, but I don't know which environment we should check especially outdated operating systems/SDKs/develop environments. Anyone Help? Or what should I do anything else?

I checked by attached .cpp code (simply replace with "minimal.cpp") with:

  1. Windows 8 Pro 64bit
    2A) Visual C++ 2010 Express (with Windows SDK 7.1 for 64 bit)
    2B) Visual Studio 2012 Express for Windows Desktop
    note1) Both 2A and 2B, I tested only UNICODE build, but both 32bit and 64bit.
    note2) All 1, 2A, 2B are working as Japanese Edition.
@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 6, 2013

2013-04-06 13:37:30: suzumizaki (Suzumizaki-Kimitaka) uploaded file wx_trunk_msw_stacktrace_20130406.patch (25.2 KiB)

Patchs to trunk to resolve stack tracing problem with non-ASCII identifiers with wxMSW

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 6, 2013

2013-04-06 13:41:21: suzumizaki (Suzumizaki-Kimitaka) uploaded file stacktracetest.cpp (7.3 KiB)

C++ code for test. Please replace minimal.cpp, save as utf-8 or utf-16, build debug mode, and push button and check whether dialog can show "日本語の関数".

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 6, 2013

2013-04-06 15:29:16: @vadz commented


Thanks for working on this. I hoped to apply it quickly but got mired in the differences between debug help API versions and also realized that we're doing some needlessly complicated things with 64 bit support so after spending almost an hour on this I'm still not done :-( Will try to finish at some later time.

In the meanwhile, it would be nice to have a patch to the except sample adding a function with non-ASCII name to be able to check how it works directly here.

TIA!

2013-04-06 15:29:16: @vadz changed status from new to accepted

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 8, 2013

2013-04-08 14:40:55: suzumizaki (Suzumizaki-Kimitaka) commented


Sorry, I found the patch 1st posted seems not working XP and Vista.
I try to make new one, please wait...

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 8, 2013

2013-04-08 14:48:26: @vadz commented


I've already done local modifications, I'll make them available later tonight, either by attaching my patch here or, preferably, by pushing out this branch to a public git repo -- do you use git?

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 8, 2013

2013-04-08 16:15:23: @vadz changed status from accepted to new

2013-04-08 16:15:23: @vadz commented

Sorry, after looking at this one again I have to give up. There are 2 big problems here:

  1. The patch mixes up 32/64 and Unicode-related changes, I'd really like to untangle them.
  2. The patch assumes that we can decide whether we use ANSI or Unicode functions at compile-time which is just not the case as it depends on the version of "dbghelp.dll" we load during run-time. So the code should be ready to work with either ANSI or Unicode functions.

I hoped to fix the former but it doesn't make much sense without fixing the latter and I won't have time to do it. So I've just checked in the 2 small changes that are uncontroversial and will leaving the rest to you...

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 8, 2013

2013-04-08 16:15:31: @vadz commented


(In [73792]) Support Unicode module names in wxDynamicLibrary::MSWGetModuleHandle().

The module names are not necessarily ASCII strings, so use wxString instead of
"char*" and W-version of GetModuleHandle() if appropriate.

See #15138.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 9, 2013

2013-04-09 05:55:10: suzumizaki (Suzumizaki-Kimitaka) commented


Thank you quick reply and sorry again my late response.
I know the problem you told, and I will try again...

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 11, 2013

2013-04-11 14:00:01: suzumizaki (Suzumizaki-Kimitaka) commented


I made new patch here now:

  1. I tested with samples/except which I also fixes this time. Try the menu "File" - "Show dialog using Unicode named class".
    2a) I use Visual C++ 2010 Express for 32bit binary,
    2b) and Windows SDK 7.1 for 64bit binary.
  2. Both tested Unicode build and not Unicode build.
  3. Both tested 32bit .exe and 64bit one.
  4. Also Debug and Release build, but partly nonsense with Release one.
    6a) Tested with Windows XP Home Edition SP3 32bit version
    6b) Windows Vista Home Premium SP2 64bit, with both 32/64bit .exe
    6c) Windows 8 Pro 64bit, with both 32/64bit .exe
  5. I also test some with Visual Studio Express for Desktop, but only with Windows 8.
  6. Above all I used Japanese edition if exists.

...HAHAHA, I have learned a bit how great daily maintainers do their works!

But some light issues remain:
a) For using with older Windows SDKs, we possibly have to define structs which only new SDKs do.
b) With 32bit .exe, SymGetLineFromAddrW64 seems to return FALSE even when the function itself is found. But I added fail safe code, this problem is not critical.
c) While converting wide char to mbcs, last a few characters cut off with some case. But this behavior depends on wxConvLocal.

Thank you!

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 11, 2013

2013-04-11 14:01:02: suzumizaki (Suzumizaki-Kimitaka) uploaded file wx_trunk_15138_20130411_all.patch (77.7 KiB)

Includes include/.h, src/.cpp, and samples/except fixes.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 11, 2013

2013-04-11 14:01:26: suzumizaki (Suzumizaki-Kimitaka) uploaded file wx_trunk_15138_20130411_core_only.patch (36.1 KiB)

Only include/.h and src/.cpp fixes, included _all version.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 11, 2013

2013-04-11 14:01:50: suzumizaki (Suzumizaki-Kimitaka) uploaded file wx_trunk_15138_20130411_except_sample_only.patch (41.6 KiB)

Only samples/except fix, included _all version.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 11, 2013

2013-04-11 14:15:02: suzumizaki (Suzumizaki-Kimitaka) commented


Logically, the patch should be _20130411_all version because samples/except enhancement requires core fix. Last 2 patches are only for convenience.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 18, 2013

2013-04-18 14:57:26: suzumizaki (Suzumizaki-Kimitaka) uploaded file 20130417_for_library.patch (35.9 KiB)

New patch for fix stack tracing codes

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 18, 2013

2013-04-18 14:58:16: suzumizaki (Suzumizaki-Kimitaka) uploaded file 20130417_for_samples_except.patch (41.8 KiB)

New patch for test stack tracing (activated only with Visual C++)

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 18, 2013

2013-04-18 15:04:40: suzumizaki (Suzumizaki-Kimitaka) commented


I make new patch to make sure building library without errors with another toolkits.

With this patch, I can build library and samples/except with:

  1. TDM_GCC 4.7 included in Code::Blocks 12.11 with mingw.
  2. TDM_GCC 4.7 64bit edition (tdm64-gcc-4.7.1.3.exe)
  3. i686-w64-mingw32-gcc-4.6.3-2-release-win64_rubenvb.7z
  4. (vc90) Windows Server 2008 and .Net FrameWork SDK
  5. (vc100) Visual C++ Express 2010 SP1(32bit) & Windows SDK 7.1(64bit)
  6. (vc110) Visual Studio Express 2012 for Desktop (of which C++ part)
    a) With gcc(1 to 3 above), Debug & Release / Unicode & ANSI build (means 2x2 patterns) succeeded.
    b) With VC++(4 to 6, contrary), 32bit & 64bit / Debug & Release / Unicode & ANSI build (means 2x2x2 patterns) succeeded.
    c) Without VC++, the new features of samples/except are simply disabled. Just compiled without errors.
    d) I built them above on Windows 8 Pro 64bit and run the samples/except.exe-s successfully on Windows XP Home 32bit SP3 and Vista Home Premium 64bit SP2.

Notes, Toolkits I can't build with external reasons:
n1) I can't build with x86_64-w64-mingw32-gcc-4.8.0-win64_rubenvb.7z (mingw-w64 includes gcc 4.8).

The linker shows a lot of FOLDERID_*** multi defined issues.

n2) Also I can't with Windows Server 2003 SP1 SDK and R2 one.

Because the toolkit can't compile variadic macros, used in include/wx/cpp.h.
The cl.exe included with this SDK(cl.exe 14.00.40310.41, vc80) seems last version that can't compile variadic macros.

Visual C++ 2005 (cl.exe 14.00.5xxxx.yy, vc80) looks have the capability but I can't install my Windows 8 Pro 64bit.

n3) Also not with more old SDK, like "Microsoft Platform SDK 2002 July"

I can't install my Windows 8 Pro 64bit. The sdk detects 64bit Windows and try to run 64bit installer, but the installer supports itanium only.

I think I did all what I can, please anyone help!

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Apr 20, 2013

2013-04-20 14:15:23: @vadz changed status from new to accepted

2013-04-20 14:15:23: @vadz commented

Thank you for all your work, I'll try to look at the new patch a.s.a.p.

I probably won't apply the changes to the sample but I'll test with your original test case (stacktestcase.cpp before committing).

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 13, 2013

2013-06-13 02:57:29: @vadz commented


I applied the patch to the latest sources and pushed the result to Github.

Could more people please test this patch please to ensure that it compiles/works correctly with all the different dbghlp.dll versions out there?

Thanks again for your work on this!

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Sep 15, 2013

2013-09-15 01:56:25: @vadz commented


Well, if nobody else has tested it since 3 months, it seems unlikely to happen at all, so I'm going to commit it finally as otherwise the patch is just going to bit rot.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Sep 15, 2013

2013-09-15 02:16:35: @vadz changed status from accepted to closed

2013-09-15 02:16:35: @vadz set resolution to fixed

2013-09-15 02:16:35: @vadz commented

(In [74817]) Make wxMSW stack walking methods work with Unicode identifiers.

This allows to show the stack properly for e.g. Japanese programs.

Closes #15138.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Sep 15, 2013

2013-09-15 13:57:24: @vadz commented


(In [74820]) Revert "Make wxMSW stack walking methods work with Unicode identifiers."

This reverts 74817 because it broke compilation with VC8 and it doesn't seem
obvious to fix this.

See #15138, closes #15500.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Sep 15, 2013

2013-09-15 14:00:16: @vadz changed status from closed to reopened

2013-09-15 14:00:16: @vadz changed resolution from fixed to **

2013-09-15 14:00:16: @vadz commented

Unfortunately less than one day of testing in the trunk was enough to find a big problem with this patch: it breaks compilation with VC8 (and certainly VC7 too), see #15000. It would be fine if the Unicode functionality was not available with the older SDKs used by these compilers but it is wrong to completely break compilation with them.

The patch will either have to be reapplied once support for them is officially dropped (not in 3.0 and probably not in 3.2 for VC8) or updated to avoid using PENUMLOADED_MODULES_CALLBACKW64 and other problematic symbols with them.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 16, 2015

2015-06-16 13:37:14: suzumizaki (Suzumizaki-Kimitaka) commented


Hello, I'm back!

I make the new patch and pull-request on GitHub!
I succeeded to patch against VC8(and VC7 seems automatically
avoid the code by API_VERSION_NUMBER even with the previous
patch).

Please accept this one!

#38

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 19, 2015

2015-06-19 00:15:40: @vadz changed status from reopened to accepted

2015-06-19 00:15:40: @vadz commented

I'm looking at this but there is still some work to do, if only to undo the incorrect merge of src/msw/dlmsw.cpp (you seem to have simply overwritten the latest version with your changes), but also other minor things.

I wonder how did you test this exactly with MinGW, do you mean that you could solve #2807 and make this stuff work with it?

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 19, 2015

2015-06-19 06:16:51: suzumizaki (Suzumizaki-Kimitaka) commented


(About 'exact' MinGW)
I don't test with 'exact' MinGW, only Mingw-w64 and TDM-GCC. the TDM-GCC is included codeblocks-13.12mingw-setup.exe. In fact, I know only recently that 'exact' MinGW is different from another two.

(About samples/debugrpt)
I had not known #2807 before you tell me above.
It seems for me some another issue. I tried to fix 'samples/except' 2 years ago, but not 'samples/debugrpt'.

(About current 'master' on GitHub)
Well, continues from http://wxwidgets.blogspot.com/2015/06/build-fixes-for-different-gcc.html , Building with MSVC++ 2005 Express and SDK for Windows Server 2003 SP1 looks fine. Clean and (re)build makes no errors like I reported. The clean build is important I know :) but it takes a lot of time as you know :(

In fact, current 'master' on GitHub cannot be built with UNICODE=0 due to another reason. I will report on wxTrac(as another issue) later.

Now I will trying build with my patch. It may take several hours or days...

Thanks!

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 21, 2015

2015-06-21 07:24:10: suzumizaki (Suzumizaki-Kimitaka) commented


I finished to rebuild. see my new report on the GitHub(#38).

Also, I reported the issue about UNICODE=0, see #17033.

I can't find the dependencies between this issue and #2807.

Thanks and please continue.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 21, 2015

2015-06-21 15:29:25: @vadz commented


Sorry, I've changed quite a few things locally by now so I'm not sure if I'm going to restart with the new versions of the PR. I'll try to finish the changes today...

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 21, 2015

2015-06-21 16:01:36: suzumizaki (Suzumizaki-Kimitaka) commented


That's no problem, because my part is not changed.
As written in GitHub, I updated to catch up the master branch only.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 24, 2015

2015-06-24 02:36:43: @vadz commented


I've pushed my version of your patch (with a few other simple fixes) to https://github.com/vadz/wxWidgets/tree/unicode-debughlp, could you please test it and let me know if it still actually works correctly with the non-ASCII symbols?

TIA!

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 24, 2015

2015-06-24 10:46:50: suzumizaki (Suzumizaki-Kimitaka) commented


Ok, I will start test it from now on, please wait...

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 25, 2015

2015-06-25 00:51:52: suzumizaki (Suzumizaki-Kimitaka) commented


  1. 'DLL Debug' and 'DLL Release' seems always fail with Visual C++
10>     ライブラリ ..\..\lib\vc_x64_dll\wxmsw31u_core.lib とオブジェクト ..\..\lib\vc_x64_dll\wxmsw31u_core.exp を作成中
10>window.obj : error LNK2019: 未解決の外部シンボル "unsigned long __cdecl wxGlobalSEHandler(struct _EXCEPTION_POINTERS *)" (?wxGlobalSEHandler@@YAKPEAU_EXCEPTION_POINTERS@@@Z) が関数 "int `__int64 __cdecl wxWndProc(struct HWND__ *,unsigned int,unsigned __int64,__int64)'::`1'::filt$0" (?filt$0@?0??wxWndProc@@YA_JPEAUHWND__@@I_K_J@Z@4HA) で参照されました。
10>..\..\lib\vc_x64_dll\wxmsw31u_core_vc_x64_custom.dll : fatal error LNK1120: 1 件の未解決の外部参照
  1. Definition of 'UNICODE=0' seems always to cause failure with Visual C++
debughlp.cpp
..\..\src\msw\debughlp.cpp(707) : error C2065: 'wxEnumLoaded64Callback' : 定義されていない識別子です。
  1. failed with UNICODE=1, VC++ 2005 Express, PSDK for Windows Server 2003 SP1
debughlp.cpp
..\..\src\msw\debughlp.cpp(695) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACKW64,PVOID)' : 2 番目の引数を 'wxPENUMLOADED_MODULES_CALLBACK' から 'PENUMLOADED_MODULES_CALLBACKW64' に変換できません。(新しい機能 ; ヘルプを参照)
        この変換には reinterpret_cast, C スタイル キャストまたは関数スタイルのキャストが必要です。

Wow... Please tell me why your fix is required against my patch...
All these errors don't reported before your fix...
Or did I something wrong?

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 25, 2015

2015-06-25 04:15:28: @vadz commented


Replying to [comment:24 suzumizaki]:

  1. 'DLL Debug' and 'DLL Release' seems always fail with Visual C++

Thanks, this was broken by an unrelated change, fixed now.

  1. Definition of 'UNICODE=0' seems always to cause failure with Visual C++

Oops, sorry, last minute change broke this, fixed too now.

  1. failed with UNICODE=1, VC++ 2005 Express, PSDK for Windows Server 2003 SP1

I don't have this compiler any more, but I guess it's due to the first argument non const-ness, right? If so, this should be fixed too now.

Wow... Please tell me why your fix is required against my patch...

As I said, I couldn't apply the patch without changes because of the bad merge of src/msw/dlmsw.cpp (you basically threw away all the changes to this file since the last 2 years instead of really merging them). And as long as I was looking at the patch anyhow, I decided to simplify it because, as I hope you agree, the old code with all of its copying between SYMBOL_INFO and SYMBOL_INFOW was very messy and the new version is much cleaner. It's unfortunate that some things have broken in the process but it's not the worst thing in the world (as long as they're fixed).

Could you please retest the new version in the same branch as in the commment:22? Not necessarily all the compilation cases (although testing with VC8 would be welcome), but whether the code actually works with Japanese (or other non ASCII) identifiers? I don't have any test code for this and this is what worries me the most.

TIA!

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 27, 2015

2015-06-27 01:17:31: suzumizaki (Suzumizaki-Kimitaka) uploaded file except_sample.zip (190.6 KiB)

New sample code and pictures for reference

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 27, 2015

2015-06-27 01:41:43: suzumizaki (Suzumizaki-Kimitaka) commented


Ok I know, thank you to take your time to explain.

but whether the code actually works with Japanese (or other non ASCII) identifiers?
I don't have any test code for this and this is what worries me the most.

If you have a time, please check new attachment zip posted just now.
This is almost same as 20130417_for_samples_except.patch with comment 8,
but all identifiers are replaced with '\uXXXX' form.
And I append the pictures, how the dialogs should be shown.

In fact, encoding of source codes are optional even in C++11(also even with UTF-8),
but all compilers should accept \uXXXX form that meets C++98/03/11 or later.

Could you please retest the new version in the same branch as in the commment:22?

I'm testing now... Currently, all of below seems fine.

  • Visual Studio 2013 Community build with .sln file
    • means 8 patterns Debug/Release, DLL Debug/DLL Release, Win32/x64
    • (all implied UNICODE=1, UNICODE=0 is not tested)
  • Visual C++ 2005 Express with PSDK for Server 2003 SP1
    • Debug Win32/Release Win32 with wx_vc8.sln
    • Debug Win32/Release Win32 with UNICODE=0 with nmake on command prompt
    • wx_vc8.sln file doesn't have the dependencies information for DLL Debug/DLL Release builds,
      but repeatedly rebuilding all projects will make success.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jun 28, 2015

2015-06-28 00:24:39: suzumizaki (Suzumizaki-Kimitaka) commented


And following configures succeeded to build with no errors:

  • Visual Studio 2013 Community, with nmake on command prompt
  • DLL Debug x86 UNICODE=0/DLL Debug x64 UNICODE=0
  • TDM-GCC included in Code::Blocks 13.12
  • Debug/Release
  • MinGW-w64(x86_64-5.1.0-win32-seh-rt_v4-rev0)
  • Debug/Release
  • MinGW "classic"
  • Debug/Release
  • ...but only with "classic", I don't know how to build the application.

@wxtrac
Copy link
Collaborator Author

@wxtrac wxtrac commented Jul 27, 2015

2015-07-27 04:01:10: @vadz changed status from accepted to closed

2015-07-27 04:01:10: @vadz changed resolution from ** to fixed

2015-07-27 04:01:10: @vadz commented

In 28587c9:
Add support for Unicode to wxStackWalker.

Use wide-char versions of debug help functions if available, falling back to
the narrow char ones otherwise.

Also improve 64 bit support by using 64 bit versions of the functions if
available as well.

Closes #15138.

@wxtrac wxtrac closed this Jul 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant