-
Notifications
You must be signed in to change notification settings - Fork 497
Add custom kernel name demangler #9134
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: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Soham Donwalkar <[email protected]>
Signed-off-by: Soham Donwalkar <[email protected]>
donwalkarsoham - is not a collaborator |
Can one of the admins verify this patch? |
Signed-off-by: Soham Donwalkar <[email protected]>
donwalkarsoham - is not a collaborator |
{ | ||
//Check if mangled prefix "_Z" is present and length is greater than mangled_prefix_length | ||
if (mangled.size() <= mangled_prefix_length || mangled.substr(0, mangled_prefix_length) != "_Z") | ||
return "Not a mangled name"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should throw instead of returning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please throw instead of return on error
std::unique_ptr<char, decltype(&std::free)> demangled_name | ||
(abi::__cxa_demangle(mangled_name.c_str(), nullptr, nullptr, &status), std::free); | ||
if (idx >= s.size()) | ||
return "?"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an error condition? If so, we should throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, added throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to use GCC style mangling and write our own Windows mangler / demangler that follows the GCC style? I am asking because it would be useful to have elf files be compatible with available tools, e.g. c++filt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
|
||
|
||
// map of mangled argument types to their demangled string representations | ||
static const std::map<char, std::string> demangle_type_map = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: initialization of 'demangle_type_map' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
static const std::map<char, std::string> demangle_type_map = {
^
Additional context
/usr/include/c++/13/bits/stl_map.h:239: possibly throwing constructor declared here
map(initializer_list<value_type> __l,
^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clang-tidy warning is valid. Let's move this static std::map inside a function that implements the lookup.
std::string
get_demangle_type(char c)
{
static std::map<...> ...;
auto it = demangle_type_map.find(c);
return (it != demangle_type_map.end())
? it->second
: "unknown";
}
Yes, AIEBU and with this PR XRT will be using Itanium ABI style mangling which is a GCC style mangling independent of the platform (Windows and Linux). I checked online, c++filt supports demangling for Itanium ABI style mangling on Windows and Linux. If the windows system has a GCC or Clang installation on Windows (e.g., through MinGW-w64 or Cygwin) then it will have c++filt. I checked on Linux using c++filt and and it was able to demangle correctly. This below mangling style is used by AIEBU and present in ELF. Note - AIEBU needs some change wrt to scalar types like int, char, uint32_t, etc. which I will be making after this PR gets merged. % c++filt _Z3DPUPvPcPi |
Okay, that comment made no sense. We are only implementing demangle, mangling is in aiebu and is GCC style. |
|
||
|
||
// map of mangled argument types to their demangled string representations | ||
static const std::map<char, std::string> demangle_type_map = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clang-tidy warning is valid. Let's move this static std::map inside a function that implements the lookup.
std::string
get_demangle_type(char c)
{
static std::map<...> ...;
auto it = demangle_type_map.find(c);
return (it != demangle_type_map.end())
? it->second
: "unknown";
}
static std::string | ||
demangle(const std::string& mangled_name) | ||
demangle_arg_type(const std::string& s, size_t& idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this function. Leverage get_demangle_type and pass the char to get the type for. Leave string (s) and index (idx) with caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our discussion offline, made the changes.
// Append "*" for pointer types | ||
if (c == 'P') | ||
return demangle_arg_type(s, idx) + "*"; | ||
|
||
auto it = demangle_type_map.find(c); | ||
return (it != demangle_type_map.end()) ? it->second : "unknown"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fold into get_demangle_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our discussion offline, made the changes.
return demangle_arg_type(s, idx) + "*"; | ||
|
||
auto it = demangle_type_map.find(c); | ||
return (it != demangle_type_map.end()) ? it->second : "unknown"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does the caller deal with "unknown"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the function args will have unknown in it. I can throw and error even if one of the args are not mangled correctly.
Sorry if my response was confusing. Let me rephrase it. In this PR we are implementing demangling in XRT matching the same style which is again following GCC style(Itanium Style). |
Haha, my " didn't make sense" comment was comment to my own comment, not to yours which I hadn't seen when I wrote mine. :-) |
Please sign-off your commit(s) |
Haha ok, got it :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
if (it == demangle_type_map.end()) | ||
throw std::runtime_error("Unknown type character in mangled name: " + std::string(1, c)); | ||
|
||
return it->second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: misleading indentation; statement is not part of the previous 'if' [clang-diagnostic-misleading-indentation]
return it->second;
^
Additional context
src/runtime_src/core/common/api/xrt_module.cpp:453: previous statement is here
if (it == demangle_type_map.end())
^
if (it == demangle_type_map.end()) | ||
throw std::runtime_error("Unknown type character in mangled name: " + std::string(1, c)); | ||
|
||
return it->second; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ident?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
clang-tidy review says "All clean, LGTM! 👍" |
while (idx < mangled.size()) | ||
args.push_back(get_demangled_type(mangled, idx)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
while (idx < mangled.size()) | |
args.push_back(get_demangled_type(mangled, idx)); | |
while (idx < mangled.size()) | |
args.push_back(get_demangled_type(mangled[idx++])); |
or simpler maybe:
for (auto c : mangled)
args.push_back(get_demangled_type(c));
static std::string | ||
demangle(const std::string& mangled_name) | ||
get_demangled_type(const std::string& s, size_t& idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this function need s
and idx
.? Can't it just take char c
. It feels wrong that this function updates idx
. I imagine caller should do get_mangled_type(name[idx++])
Signed-off-by: Soham Donwalkar <[email protected]>
Signed-off-by: Soham Donwalkar <[email protected]>
Signed-off-by: Soham Donwalkar <[email protected]>
clang-tidy review says "All clean, LGTM! 👍" |
Problem solved by the commit
XRT was separate demangler APIs for Linux (GCC API) and Windows (UnDecorateSymbolName) while AIEBU has mangling implementation similar to the one on Linux.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
N/A
How problem was solved, alternative solutions (if any) and why they were rejected
_Z<length><name><types>
Risks (if any) associated the changes in the commit
What has been tested and how, request additional testing if necessary
1> Successfully ran End to end test on aie4 on Windows and Linux as given below. (It also prints the mangled and demangled name)
1. Test on aie4 Windows with 3 args-
2. Test onaie4 Linux with 3 args
3. Test onaie4 Linux with no arg
4. Test on aie2p Windows
5. Test on aie2p Linux
Documentation impact (if any)
N/A