comgr-objdump.cpp causes crash if loaded alongside mesa #52
Comments
|
Here is Blender 3.4 bug report with more details about the same issue: |
|
I still don't understand why either mesa or comgr register these command line options. Does anyone know what their purpose is? |
|
comgr-objdump seems to be a derivate from an old version of https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-objdump/llvm-objdump.cpp, the original file is intended as a command-line utility and it makes sense to define command-line options. The conflicting definition seems to be in https://github.com/llvm/llvm-project/blob/239a174d92dd2bd99ecb1308dc8937040895b04d/llvm/lib/Support/CommandLine.cpp#L2618 |
|
cc @lamb-j |
|
Ideally we could have two separate threads/components both manage their own options without clashing, but this is a known shortcoming of the way LLVM options are currently implemented. We have ambitions to work on this in the long term, but it requires big upstream LLVM changes. In the short term, I think it may be safe to remove some alias options from comgr-objdump that have a higher probability of clashing with other libraries (like "-h" here). @kzhuravl Maybe we could remove all of the short/shorter options (-d, -p, -h, -s, etc.) and document that comgr-objdump requires full-length option names? FYI these options can be set by a user via amd_comgr_action_info_set_option_list(...) before calling something like amd_comgr_do_action(AMD_COMGR_ACTION_DISASSEMBLE_RELOCATABLE_TO_SOURCE, ...) |
|
How did you install the patch? I tried to do the same thing, but got these error messages: |
It looks like on Gentoo the archive does not have the same layout. The patch should be diff -ru ROCm-CompilerSupport-rocm-5.3.3.orig/src/comgr-objdump.cpp ROCm-CompilerSupport-rocm-5.3.3/src/comgr-objdump.cpp
--- ROCm-CompilerSupport-rocm-5.3.3.orig/src/comgr-objdump.cpp 2022-07-28 17:06:14.000000000 +0200
+++ ROCm-CompilerSupport-rocm-5.3.3/src/comgr-objdump.cpp 2022-12-20 20:03:53.357398486 +0100
@@ -175,9 +175,6 @@
static cl::alias SectionHeadersShort("headers",
cl::desc("Alias for --section-headers"),
cl::aliasopt(SectionHeaders));
-static cl::alias SectionHeadersShorter("h",
- cl::desc("Alias for --section-headers"),
- cl::aliasopt(SectionHeaders));
cl::list<std::string>
FilterSections("section",The difference is only in the file path ( |
|
@xytovl Thanks for the help! |
Which changes might that be and are they addressed by the following reviews? I happen to have approached this issue from the LLVM side with |
|
@xytovl I just committed 2d05f9e Thanks for tracking this down! @labre Nicolai's patches and the removal of ManagedStatic definitely addresses some of the issues. I haven't sat down with them yet to see what still needs to happen. Besides examples like this where we have Comgr and Mesa running at the same time, we also would like the option of having multiple instances of Comgr (and thus multiple LLVMs) running without hitting issues with the option parser. I'll let you know if I figure anything out on this front, and I'd love to be kept in the loop on your progress! |
That would be taking false credit for others work. I’m just a Gentoo user, who simultaneously tried to track down this in LLVM instead of RocM. I just tested a quick hack in LLVM and then opened a RFC on this to increase awareness for the CommandLine parser issues in practice. However I eventually was hinted towards this bug by gdb and the ongoing progress in LLVM by another discourse user, so basically I was a tad too late for investigating this one. ;) Since @nhaehnle is the author of the Revisions, he is the better contact for staying in the loop. Thanks for merging the fix. :) |
You said you come from Gentoo. Have you been able to test this somehow? I'm hitting a similar problem with Blender crashing due to ROCm getting in conflict with MESA and the ROCm version 5.3.3 seems not to be new enough. |
Well, I used the line deletions from the commit as a user patch and forgot about it. I updated it to be the output of
You can use the user patch attached with this post and put it into /etc/portage/patches/dev-libs/rocm-comgr as follows and as root, but please compare it with the commit diff. Do not ever trust a random patch offered to you on the Internet and double check the link targets of this post: 0001-Remove-h-option-from-comgr-objdump.md Let me know, whether this works for you. If you have any questions feel free to ask. Otherwise you’re welcome. :) Oh, and since you seem to not know the user patches feature, I suppose you’re relatively new to the Gentoo world. In that case, welcome to the community! :) |
|
Actually I use Gentoo since nearly 20 years but I never got in contact with user patches so far. |
|
This is the modified version of the patch i use myself on Gentoo with the path changes done. Drop in diff -ru comgr.orig/src/comgr-objdump.cpp comgr/src/comgr-objdump.cpp
--- comgr.orig/src/comgr-objdump.cpp 2022-07-28 17:06:14.000000000 +0200
+++ comgr/src/comgr-objdump.cpp 2022-12-20 20:03:53.357398486 +0100
@@ -175,9 +175,6 @@
static cl::alias SectionHeadersShort("headers",
cl::desc("Alias for --section-headers"),
cl::aliasopt(SectionHeaders));
-static cl::alias SectionHeadersShorter("h",
- cl::desc("Alias for --section-headers"),
- cl::aliasopt(SectionHeaders));
cl::list<std::string>
FilterSections("section", |
When using both libamd_comgr.so and mesa with libvulkan_radeon, application (blender-3.3 in my case) crashes with
the following patch fixes this for me:
If I understand correctly, none of the command line options and aliases defined here are used, but this one alone fixes the issue.
The text was updated successfully, but these errors were encountered: