As discussed in https://reviews.llvm.org/D138595 - the current demangler API
can't really report more detailed errors. This is an attempt to implement
that using std::optional and std::error_code
Details
- Reviewers
dblaikie jhenderson MaskRay
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
60,040 ms | x64 debian > libFuzzer.libFuzzer::value-profile-load.test |
Event Timeline
@dblaikie @jhenderson This is an attempt to implement the API discussed in the other Diff using std::error_code and std::optional. Let me know what you think before I convert the rest of the methods to have similar error handling.
I also think the demangle() and nonMicrosoftDemangle() should probably follow the same API if we decide this is what we want.
llvm/lib/Demangle/Demangle.cpp | ||
---|---|---|
76–79 | Part of what I was hoping for was that we wouldn't need an explicit isItaniumEncoding check, even inside the implementation - that Demangler::parse could produce that as a separate error result if it parsed something without the desired prefix. Would that be possible? |
llvm/include/llvm/Demangle/Demangle.h | ||
---|---|---|
27 | I generally support the use of enum class, but the enums are generally being cast to an int, so maybe that's a sign that this should just be a plain enum? | |
28 | Nit: comments should end with "." (here and below). | |
53 | Nit: capitalization here is inconsistent. FWIW, the LLVM coding standards state for warning and error messages to a) start with a lower case letter, and b) not to end with a period. Whether that applies in this context is debatable. | |
66 | I'd just overload the function name, rather than appending "WithError". The fact that it takes a std::error_code reference indicates that it will do something to that field, and it's then similar to e.g. how std::filesystem overloads function. | |
llvm/lib/Demangle/Demangle.cpp | ||
76–79 | Not sure if this is what @dblaikie is suggesting, but it seems to me that you could have the underlying demangling code set a status of "NotThisFormat" or similar if it encounters something with the wrong prefix type. This would enable calling code to know whether to try the next format, or just stop. That might not work so well in the face of type demangling though. |
Oh, and just to mention that this week is my last week working before a 6 week break, so I may not have the time to review/comment much more on this.
llvm/include/llvm/Demangle/Demangle.h | ||
---|---|---|
27 | Ideally we should change the api to not return a int status from the demangling function instead using DemangleError() - as I noted in my other comment, I was trying to preserve the old API because I thought it was wide-spread. But that doesn't seem to be the case, so I might end up just fixing that instead of making this a normal int. | |
53 | Hmm seems like it should apply here, I will make it consistent. | |
66 | Agreed. | |
llvm/lib/Demangle/Demangle.cpp | ||
76–79 | We can do that - but it becomes a little more complex and we probably need to audit all the places where the demangling functions are used now in that case. itaninumDemangle() calls: AbstractManglingParser::parse() which returns nullptr if there is any errors, but have no status reports here. So we could either:
I was going to say that it's harder to change the API of itaniumDemangle since it's used "everywhere" - but that doesn't seem to be true - searching for it only seems to happen in 10 places: I am going to give it a try to move the logic to AbstractManglingParser to return a error_code instead and see if it can work. |
Hi - I finally got around to picking this one up again. This is rebased, passing all tests and now we pass the std::error_code directly to the API and changed all the callers to itaniumDemangle() and changed most of the places to actually check the error_code instead of the return const char*.
What do you guys think about this API change? If we like this in general, I can start changing the other demangle calls, either as part of this diff or in separate diffs per demangle API.
Adding std::error_code looks good.
This is an attempt to implement that using std::optional and std::error_code
I don't see std::optional in the patch :)
llvm/include/llvm/Demangle/Demangle.h | ||
---|---|---|
53 | If there isn't a precedent, as mentioned, no-capitalization is preferred (https://llvm.org/docs/CodingStandards.html#error-and-warning-messages). Also, no-capitalization makes the message easily composable with a function that appends a prefix. |
Could make the error_code out parameter optional (take by pointer, populate if non-null, have a null default parameter value) - then callers who don't benefit from/care about the specific type can continue using the null pointer result signal?
llvm/lib/ProfileData/ItaniumManglingCanonicalizer.cpp | ||
---|---|---|
290–296 | This could be changed to parse, check the error and then do the make<NameType>(StringView thing if the parse failed as invalid, maybe? (potentially in a follow-up commit) |
I generally support the use of enum class, but the enums are generally being cast to an int, so maybe that's a sign that this should just be a plain enum?