[PATCH] D111414: [Demangle] Add minimal support for D programming language

[Luís Ferreira via Phabricator via llvm-commits]

ljmf00 added inline comments.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:36
+/// \note Beware these aren't required to be '\0' terminated
+struct OutputString {
+
----------------
dblaikie wrote:
> dblaikie wrote:
> > ljmf00 wrote:
> > > jhenderson wrote:
> > > > ljmf00 wrote:
> > > > > dblaikie wrote:
> > > > > > jhenderson wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > ljmf00 wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > Any chance of using an existing stream type (like `llvm::itanium_demangle::OutputStream` which `RustDemangle` also uses?)? Otherwise might be worth a bunch of separate testing of this class - or incrementally adding functionality to it that functionality is used in the patch series, otherwise it's hard to tell that everything's tested if it's added in one go here & significant parts are currently unused.
> > > > > > > > > This type differs from a stream because D demangler needs the ability to prepend to the output string due to how D demangling is designed. Because of that, a stream is not a good fit here and plus adding methods like prepend will make it conceptually not a stream at all.
> > > > > > > > > 
> > > > > > > > > I will incrementally add parts of it when necessary and add tests for it. Although, if you find any other data structure that might be suitable here, please let me know.
> > > > > > > > Ah, fair enough. Might want to check the naming conventions (I'd expect "need" to be called "reserve" in C++ API parlance, though perhaps in LLVM's APIs that's called "grow"?)/API design (do the other similar data structures have a "free()" function, or do they rely on the dtor to cleanup?) line up with the existing OutputStream/other data structures in LLVM for consistency.
> > > > > > > > 
> > > > > > > > & maybe worth pulling it out into a separate header - given how big this whole file is likely to get?
> > > > > > > I wonder if we could model this as some kind of `deque<std::string>` - basically buffer things by adding strings to the start or end of the queue (prepend and append), before finalizing it into a single string at a later point. It might help avoid a lot of manual memory management/copying etc.
> > > > > > Probably would be a `deque<char>`? Though that'd then involve copying the result into the output buffer - so I think the current approach is probably a/the good one, and consistent with the other demanglers that write directly into their output buffer. Just with the added constraint of needing to be able to prepend info.
> > > > > > 
> > > > > > (another alternative would be to generalize the existing itanium OutputStream to support prepending too)
> > > > > I think a deque is not a good fit for this for several reasons. I think you were referring to some sort of `deque<char*>`, otherwise it would be very memory inefficient -- the deque node structs are way heavier than a byte.
> > > > > 
> > > > > Even with a string deque memory not being contiguous across nodes can also hurt performance, when, e.g. calculating the length of a given deque. Another problem with that is the fact that memory allocation is done in chunks and gown twice when needed.
> > > > > 
> > > > > Also, assuming that a string is appended/prepended to the deque on every OutputString::append or OutputString::prepend, allocating small chunks individually is also incredibly slow -- if that is not the case I'm not seeing a way where it is applicable at all.
> > > > > 
> > > > > Talking about the OutputStream, as I mentioned above, prepending is not part of a Stream concept (correct me if I'm wrong), since a stream is designed to only grow at the end. I'm not quite sure if something is relying on specific characteristics of a stream but, e.g. because a stream only grows at the end, adding a prepend would mess up with indices. If that is not a problem, I would consider that option but also renaming it.
> > > > Yeah, char* or string is what I was thinking, since if you don't need to go back and modify any of the already added string, you can just naturally add substrings at either end. You could also try a simple `std::list`, since that would avoid the overhead of reserving space completely. I've not really studied the performance implications of the various std containers though, so I can't really advise what's the most appropriate overall. I'm more interested in the practicality of writing and maintaining the code (don't reinvent the wheel and all that, which I feel this class more-or-less is doing).
> > > > 
> > > > I'm not sure when you need to calculate the overall length, so I'm not sure that aspect is relevant?
> > > I thought wrongly about deque. Probably deque<char> is desirable compared to deque<char*> or deque<string> since it has the same behaviour of std::vector but with amortized constant insertions at the beginning or end of the nodes. I still don't see good fit because of what I will describe bellow:
> > > 
> > > About the length, `getLength()` is being used on some parts of the code. The length on a deque is O(N), being N the number of nodes, so it is certainly less performant, unless I rely on a manual counter. The same applies with std::list, with the risk of having even more nodes on std::list, since it is internally implemented with a double linked list instead of individually allocated arrays. The only benefit std::list brings is the constant insertion in the middle. Also copying continuous memory in chunks can be way faster with optimized SIMD `memcpy`.
> > > 
> > > I read the code of `OutputStream` and it seems reasonable to me to use it, as it has a very similar approach to this implementation. The only major thing I really see is prepending. I can add the prepend method but my question remains the same, is it really a stream or should we consider renaming it? Is there any implication renaming it, if that is the case? If that is not possible, I thought about extending OutputString from OutputStream making that possible.
> > If we were going to move away from a custom data structure, to something that'll require copying at the end of the demangling (to get it into a standalone malloc'd buffer to return from the API) I'd probably suggest just using `std::string` - it's got the same general performance characteristics (eg: O(N) on prepend) as the current code, and is simple/direct, doesn't introduce potential other issues like memory fragmentation, etc.
>  Re: OutputStream: Yeah, I think it's probably fine to rename it (`OutputBuffer`, `StringBuffer`, `OutputStringBuffer`? It's already in a (perhaps overly specific (itanium, even though it's already used for Rust and now potentially for D) namespace, so doesn't have to include a "demangled" or something else to disambiguate it) - I'm OK with any of the suggested names, including `OutputString`. I guess `OutputBuffer` is closer to the original name, while removing the "only adds to the end" semantics carried by the word "stream".
> 
> Probably rename it in one patch, add the needed prepend in whatever patch is using it shortly thereafter.
The problem I mentioned earlier about `std::string`, and addressed by `OutputStream` and this `OutputString` implementation, is the fact that the `grow()`/`need()` expands twice the current size + requested size as opposed to the `reserve()` method in `std::string` (See here for more context: https://github.com/llvm/llvm-project/blob/main/libcxx/include/string#L3323). This is important because appending to `OutputStream`/`OutputString` is more efficient with significant less allocations.


================
Comment at: llvm/lib/Demangle/DLangDemangle.cpp:36
+/// \note Beware these aren't required to be '\0' terminated
+struct OutputString {
+
----------------
ljmf00 wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > ljmf00 wrote:
> > > > jhenderson wrote:
> > > > > ljmf00 wrote:
> > > > > > dblaikie wrote:
> > > > > > > jhenderson wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > ljmf00 wrote:
> > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > Any chance of using an existing stream type (like `llvm::itanium_demangle::OutputStream` which `RustDemangle` also uses?)? Otherwise might be worth a bunch of separate testing of this class - or incrementally adding functionality to it that functionality is used in the patch series, otherwise it's hard to tell that everything's tested if it's added in one go here & significant parts are currently unused.
> > > > > > > > > > This type differs from a stream because D demangler needs the ability to prepend to the output string due to how D demangling is designed. Because of that, a stream is not a good fit here and plus adding methods like prepend will make it conceptually not a stream at all.
> > > > > > > > > > 
> > > > > > > > > > I will incrementally add parts of it when necessary and add tests for it. Although, if you find any other data structure that might be suitable here, please let me know.
> > > > > > > > > Ah, fair enough. Might want to check the naming conventions (I'd expect "need" to be called "reserve" in C++ API parlance, though perhaps in LLVM's APIs that's called "grow"?)/API design (do the other similar data structures have a "free()" function, or do they rely on the dtor to cleanup?) line up with the existing OutputStream/other data structures in LLVM for consistency.
> > > > > > > > > 
> > > > > > > > > & maybe worth pulling it out into a separate header - given how big this whole file is likely to get?
> > > > > > > > I wonder if we could model this as some kind of `deque<std::string>` - basically buffer things by adding strings to the start or end of the queue (prepend and append), before finalizing it into a single string at a later point. It might help avoid a lot of manual memory management/copying etc.
> > > > > > > Probably would be a `deque<char>`? Though that'd then involve copying the result into the output buffer - so I think the current approach is probably a/the good one, and consistent with the other demanglers that write directly into their output buffer. Just with the added constraint of needing to be able to prepend info.
> > > > > > > 
> > > > > > > (another alternative would be to generalize the existing itanium OutputStream to support prepending too)
> > > > > > I think a deque is not a good fit for this for several reasons. I think you were referring to some sort of `deque<char*>`, otherwise it would be very memory inefficient -- the deque node structs are way heavier than a byte.
> > > > > > 
> > > > > > Even with a string deque memory not being contiguous across nodes can also hurt performance, when, e.g. calculating the length of a given deque. Another problem with that is the fact that memory allocation is done in chunks and gown twice when needed.
> > > > > > 
> > > > > > Also, assuming that a string is appended/prepended to the deque on every OutputString::append or OutputString::prepend, allocating small chunks individually is also incredibly slow -- if that is not the case I'm not seeing a way where it is applicable at all.
> > > > > > 
> > > > > > Talking about the OutputStream, as I mentioned above, prepending is not part of a Stream concept (correct me if I'm wrong), since a stream is designed to only grow at the end. I'm not quite sure if something is relying on specific characteristics of a stream but, e.g. because a stream only grows at the end, adding a prepend would mess up with indices. If that is not a problem, I would consider that option but also renaming it.
> > > > > Yeah, char* or string is what I was thinking, since if you don't need to go back and modify any of the already added string, you can just naturally add substrings at either end. You could also try a simple `std::list`, since that would avoid the overhead of reserving space completely. I've not really studied the performance implications of the various std containers though, so I can't really advise what's the most appropriate overall. I'm more interested in the practicality of writing and maintaining the code (don't reinvent the wheel and all that, which I feel this class more-or-less is doing).
> > > > > 
> > > > > I'm not sure when you need to calculate the overall length, so I'm not sure that aspect is relevant?
> > > > I thought wrongly about deque. Probably deque<char> is desirable compared to deque<char*> or deque<string> since it has the same behaviour of std::vector but with amortized constant insertions at the beginning or end of the nodes. I still don't see good fit because of what I will describe bellow:
> > > > 
> > > > About the length, `getLength()` is being used on some parts of the code. The length on a deque is O(N), being N the number of nodes, so it is certainly less performant, unless I rely on a manual counter. The same applies with std::list, with the risk of having even more nodes on std::list, since it is internally implemented with a double linked list instead of individually allocated arrays. The only benefit std::list brings is the constant insertion in the middle. Also copying continuous memory in chunks can be way faster with optimized SIMD `memcpy`.
> > > > 
> > > > I read the code of `OutputStream` and it seems reasonable to me to use it, as it has a very similar approach to this implementation. The only major thing I really see is prepending. I can add the prepend method but my question remains the same, is it really a stream or should we consider renaming it? Is there any implication renaming it, if that is the case? If that is not possible, I thought about extending OutputString from OutputStream making that possible.
> > > If we were going to move away from a custom data structure, to something that'll require copying at the end of the demangling (to get it into a standalone malloc'd buffer to return from the API) I'd probably suggest just using `std::string` - it's got the same general performance characteristics (eg: O(N) on prepend) as the current code, and is simple/direct, doesn't introduce potential other issues like memory fragmentation, etc.
> >  Re: OutputStream: Yeah, I think it's probably fine to rename it (`OutputBuffer`, `StringBuffer`, `OutputStringBuffer`? It's already in a (perhaps overly specific (itanium, even though it's already used for Rust and now potentially for D) namespace, so doesn't have to include a "demangled" or something else to disambiguate it) - I'm OK with any of the suggested names, including `OutputString`. I guess `OutputBuffer` is closer to the original name, while removing the "only adds to the end" semantics carried by the word "stream".
> > 
> > Probably rename it in one patch, add the needed prepend in whatever patch is using it shortly thereafter.
> The problem I mentioned earlier about `std::string`, and addressed by `OutputStream` and this `OutputString` implementation, is the fact that the `grow()`/`need()` expands twice the current size + requested size as opposed to the `reserve()` method in `std::string` (See here for more context: https://github.com/llvm/llvm-project/blob/main/libcxx/include/string#L3323). This is important because appending to `OutputStream`/`OutputString` is more efficient with significant less allocations.
Ok, I'm going to write a patch to add prepending and rename it accordingly to `OutputBuffer`. We can discuss further the implementation/approach there.


================
Comment at: llvm/unittests/Demangle/DLangDemangleTest.cpp:19
+
+TEST(DLangDemangle, Success) {
+  struct ExpectedVal {
----------------
dblaikie wrote:
> ljmf00 wrote:
> > dblaikie wrote:
> > > dblaikie wrote:
> > > > ljmf00 wrote:
> > > > > jhenderson wrote:
> > > > > > dblaikie wrote:
> > > > > > > ljmf00 wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > I think most of the mangling testing is done via llvm-cxxfilt - see llvm/test/Demangle/rust.test - so probably best this testing be done that way too.
> > > > > > > > I have a point on this that would like to take into consideration. Since these are unitary tests, I guess this has a better fit here rather than on the cxxfilt integration tests, even though majorly all the demangling tests are written there. I don't know if there is any rationale to test the behaviour on the integrations tests, but since these can all be isolated and therefore tested in a more pure way I think we should move them here, and only test if the path to this demangler is correct on cxxfilt.
> > > > > > > Generally LLVM has small enough tools/binaries that can test functionality in sufficient isolation that most testing is done via these tools (general data structures like those in llvm/ADT are tested with unit tests, for instance - and a few other places where API usage is broad/varied and testing quirks (like different kinds of error handling/corner cases/etc) is valuable).
> > > > > > > 
> > > > > > > In some ways this can be simpler to work with (eg: nothing needs to be recompiled when the test is modified, extracting a command line to run under a debugger is clear) & partly it's what LLVM developers/development are used to.
> > > > > > In my personal opinion gtest testing makes more sense for testing demangling than lit tests. If nothing else, it's probably significantly more efficient, especially on Windows, where the process launching overhead of spawning llvm-cxxfilt and FileCheck is going to be far greater than the time spent running the actual testing itself.
> > > > > If we choose to move to llvm-cxxfilt tests, I will still maintain minimal testing here, mimicking what Rust is currently doing, otherwise I will need to squash https://reviews.llvm.org/D110577 . I agree with @jhenderson for performance reasons although I don't know if it is a point strong enough for changing the testing workflow. Is that spawn process overhead considerable changing regarding the recompilation times? If the case is something like x10 slower per test, I would say that isn't worth changing to cxxfilt test suite.
> > > > Generally this'd only be a single test file with one llvm-cxxfilt invocation and one FileCheck invocation so the overhead shouldn't be too great (yeah, I'd push back against splitting it into a bunch of run lines and separate lit tests).
> > > > 
> > > > Part of my motivation here is consistency, among other reasons, for test discovery - having tests in the same place across the different demanglers, and across LLVM generally, hopefully gives us some chance of being able to maintain tests - add new test cases to the same place, rather than creating another test file because we can't find a good home to add things to.
> > > Yeah, I think we'll want both - at least a small unit test to demonstrate the API usage (as the other demanglers have, I think?) and the llvm-cxxfilt test to demonstrate that's wired up correctly too.
> > > 
> > > Then the question is where we put all the functional test cases - since we'll end up with both API and command line tests. And I'd prefer those tests live in a similar place/way to the other demanglers (& the way LLVM tends to test more broadly), which I think is in llvm-cxxfilt
> > How should I proceed here, then?
> eh, I'm coming around to it - if we've got to have both FileCheck+unit tests anyway, maybe here's a chance to experiment with/try to see if it's something better - maybe I'm just reacting to recent negative experiences struggling with FileCheck, but something more intentional like gtest's expects seem like they can produce better error messages/recovery/etc, than FileCheck. So, sure, let's go with this - figure out the data expansion issues @jhenderson has raised to keep the testing as simple/streamlined as possible. Maybe, if someone feels like it, this could be backed up with some examples of different failure experiences between the two possible approaches (especially multiple failures - showing how FileCheck probably can't recover after the first failure (without some extra output to bind to with -DAG, for instance - and thus doubling the number of CHECK lines, etc) whereas the gtest should be able to report several failures quite clearly - I wonder if gtest does a better job of highlighting differences in long similar strings? At least I think it aligns them and prints them on one line after another, unlike FileCheck)
> 
> (perhaps FileCheck could have some features that would make this sort of test situation smoother, like a "continue after CHECK-NEXT failure, skipping the failed line" and maybe in that mode, or as a separate mode, a "no, I really know this line should look like this, so if it doesn't, don't go searching for another possibly intended match, give me an exact A/B comparison with this line" (or maybe even the fuzzy line matching suggestion should be printed side-by-side with the CHECK expression, especially if the CHECK doesn't contain regex - maybe with some fancy logic to try to find where the pattern starts if it doesn't match the start of the line)
> 
> Guess that's another aspect: Do any of the existing mangler tests use any of the FileCheck features such as regex or partial line matching? If they do, then that's maybe a sign that gtest exact matching isn't a perfect fit - but I guess they probably don't, or that if they do it's not enough to matter/really sway the decision.
> Guess that's another aspect: Do any of the existing mangler tests use any of the FileCheck features such as regex or partial line matching? If they do, then that's maybe a sign that gtest exact matching isn't a perfect fit - but I guess they probably don't, or that if they do it's not enough to matter/really sway the decision.

About D demangling in specific, no. I don't know much about the other demanglers in the LLVM tree.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111414/new/

https://reviews.llvm.org/D111414