-
Notifications
You must be signed in to change notification settings - Fork 15
Fixes for Windows #21
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
base: main
Are you sure you want to change the base?
Conversation
|
@clemenswasser Thank you very much for taking the effort to do this! It seems the 'Demangle' sources you suggest adding are used in a tool I wasn't aware of - llvm-cxxfilt . I briefly considered demumble and maybe one other, but wasn't aware of an llvm demangling solution that applies beyond clang. For me it is installed alongside clang (/usr/lib/llvm-14/bin). Do you know if it's installable (as binary) on Windows/macOS? If yes, I think it is better to try and use it by default, and if not available - emit a suggestion to the user to install it. Alternatively - I know of some projects that include such external binaries they depend on in the actual repo (as binaries). Don't know yet what I think of that. |
|
Yes, using llvm-cxxfilt sounds ideal 👍 I've looked and llvm-cxxfilt gets installed with the LLVM installer on Windows under |
83d108f to
c3214cf
Compare
|
@OfekShilon I've now changed to using llvm-cxxfilt, but it crashes (only) on Windows, do you have any idea what the problem could be? |
|
@supox maybe you can help? |
|
I think I can shed some light on the issue (don't have a solution yet). https://www.pythonforthelab.com/blog/differences-between-multiprocessing-windows-and-linux/ code in PR will run fine with -j 1 switch (as no MP is available_) Since I am not an expert in Python, I currently can not tell how to solve it, besides extremely silly and obvious solution (works for me, though obviously it creates unnecessary lock for each process, and multiple instances of demangle) :) May be someone more experienced can do it better! |
|
@AntonYudintsev Thank you very much! @YoavShilon05 perhaps you can take a look too? |
|
I've been testing with this PR's patches on a fairly large project built with I did need to add a patch to replace Here is the patch that I used. Unfortunately, I'm not familiar enough with Python's |
Based on OfekShilon#21 discussion. * add empty lock for windows and start a new demangler for each process. It is arguably not the worst solution even on other platforms, as Locks are not free, and for many files it is probably better to have demangler per process, rather than Lock them * replace \\ with in generated HTMLs, to produce correct filenames on windows
|
I have made a (working for me) pull request (since I can't/don't know how amend this). Anyway,
|
|
@clemenswasser I apologize for the delay in looking into this. I just tested and merged @AntonYudintsev's PR that I believe properly works around the MP linux/windows diff. I intend also to search various demanglers (at the very least c++filt, llvm-cxxfilt and undname) at default install locations and use what is available - with fall back to non-demangling. Could you please check if the latest version works for you on windows? (you'd probably need to still pass --demangler explicitly) Are there other issues that are addressed in your PR? If yes - could you kindly separate it into smaller PRs (1 issue per PR) to ease reviewing and merging? |
|
@OfekShilon no apologies needed, thanks for merging this! @AntonYudintsev patch doesn't really work for me. |
|
@clemenswasser can you please share the exact steps you take to build cpp_optimization_example and run optview2 on it over windows? I hope to reproduce and investigate. |
|
Sure, this reproduces the exception in PowerShell: > clang++ -fsave-optimization-record '-foptimization-record-file=main.yaml' -O3 --std=c++17 main.cc
> python ..\opt-viewer.py --demangler 'llvm-cxxfilt.exe -n' .\main.yaml |
I've verified that this branch works on Windows, Linux, and macOS.
It uses the LLVM Demangle lib as a cross-platform demangler (when merged back upstream, it should be easier to require it, as it's one of LLVM's core libraries).