aadsm marked an inline comment as done. aadsm added a comment. I see what you mean, this is a good point. Yes, like you say I'm assuming the string I want to read is in a page that I can read using the fast method. And in this situation I prefer to minimize the number of calls to ReadMemory than the amount of data I'll read (given that I'll be reading from within the same page that is already cached, and also assuming the destination is also cached (probably true if in the stack)). I also wanted to side step the question of what is a good string average size because I don't have good data to back it up. So yeah, optimizing for the case I know it exists, and we can tweak this better once we have other cases to analyse.
================ Comment at: lldb/unittests/Host/NativeProcessProtocolTest.cpp:128-133 + EXPECT_THAT_ERROR( + Process.ReadCStringFromMemory(0x0, &string[0], sizeof(string), bytes_read) + .ToError(), + llvm::Succeeded()); + EXPECT_STREQ(string, "hel"); + EXPECT_EQ(bytes_read, 3UL); ---------------- labath wrote: > aadsm wrote: > > labath wrote: > > > I'm wondering how will the caller know that the read has been truncated. > > > Given that ReadMemory already returns an error in case of partial reads, > > > maybe we could do the same here ? (return an error, but still set > > > bytes_read and the string as they are now). What do you think ? > > I'm a bit hesitant on this one, because it's different semantics. > > ReadMemory returns an error when it was not able to read everything that it > > was told to. > > > > In this test case we were able to read everything but didn't find a null > > terminator. I decided to set a null terminator in this case and say that we > > read 1 less byte in order to 1) actually have a C string (I don't want > > people to strlen it and fail) and 2) allow people to read in chunks if they > > want (however, I don't know how useful this will be if at all) or allow > > them to partially read a string or even to realize that the string is > > larger than originally expected. > > > > There is a way to know if it didn't read a full string with `strlen(string) > > == bytes_read` but that'd be O(n). You can also do `bytes_read == sizeof-1 > > && string[size-2] != '\0'` but it kind of sucks. > > I have no good solution here :D unless we want to return an enum with the 3 > > different options. > > In this test case we were able to read everything but ... > Is that really true? We were told to read a c string. We did not find a c > string, so instead we chose to mangle the data that we have read into a c > string. That seems a bit error-ish. > > I also find it confusing that in this (truncated) case the returned value > corresponds with the length of the string, while in the "normal" case in > includes the nul terminator. I think it would be more natural to do it the > other way around, as that would be similar to what snprintf does. > > How about we avoid this issue entirely, and have the function return > `Expected<StringRef>` (the StringRef being backed by the buffer you pass in > as the argument) instead? This way you don't have to do any null termination, > as the StringRef carries the length implicitly. And one can tell that the > read may be incomplete by comparing the stringref size with the input buffer > size. > Is that really true? We were told to read a c string. We did not find a c > string, so instead we chose to mangle the data that we have read into a c > string. That seems a bit error-ish. That is fair. > How about we avoid this issue entirely, and have the function return > Expected<StringRef> (the StringRef being backed by the buffer you pass in as > the argument) instead? This way you don't have to do any null termination, as > the StringRef carries the length implicitly. And one can tell that the read > may be incomplete by comparing the stringref size with the input buffer size. I think my only concern with this is that people might think the passed in buffer would be null terminated after the call (this is true for snprintf). I'm not sure how to set the expectation that it wouldn't... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
