[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added a comment. sgtm Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm added a comment. I've just done these 2 steps: - Revert rL364751 (which is a revert itself) - Revert 9c10b620c061 (will re-apply this one) Should be fine since now we have an option

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

jankratochvil added a comment. In D62503#1578934 , @aadsm wrote: > Does it sound good to you? Yes, that sounds great, thanks. Sorry I am not participating these days. Repository: rL LLVM CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm added a comment. @jankratochvil I'm planning to follow Pavel's plan now that the new setting has landed and finally got some time for this. It will be: - Revert rL364751 (which is a revert itself) - Revert 9c10b620c061

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm added a comment. Yap, that makes a lot of sense to me. I looked into this over the weekend and figured out the reason why it was broken (tl;dr: we can't use LoadModules) and created a tidier version of D63868 that, like D62504

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added a comment. I've also reverted the preceeding patch in this series as reverting this one has caused one of the other tests to start failing (I've tried to make a more surgical fix first, but @jankratochvil pointed out that this basically reintroduced the problem that we were trying

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

jankratochvil added a comment. I was debugging it yesterday but I have no real result yet. There is/was a problem that initially it is run with: intern-state ProcessGDBRemote::GetLoadedModuleList intern-state parsing: intern-state found (link_map:0x77ffd990,

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

teemperor added a comment. It seems to break looking up symbols from shared libraries. A simple reproducer is debugging a simple "Hello World" program in C on Linux (Arch Linux in my case, but it seems to also affect other distributions) #include int main() { printf("Hello

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm added a comment. Reverted here: https://reviews.llvm.org/rG9c10b620c0619611dfe062216459431955ac4801 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503 ___ lldb-commits

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm added a comment. I can't repro it right now so I prefer to revert it (I'm curious how this new function could influence those tests though). I actually thought this was sorted, sorry about that. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

teemperor added a comment. @aadsm Is there any update on the regression fix? This patch also breaks the C++ module test suite, so i would prefer if we could revert it if the fix takes longer. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

jankratochvil added a comment. There is a regression on Fedora 30 x86_64: Failing Tests (25): LLDB :: tools/lldb-mi/breakpoint/break-insert-enable-pending.test lldb-Suite :: expression_command/call-function/TestCallStdStringFunction.py lldb-Suite ::

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

This revision was automatically updated to reflect the committed changes. Closed by commit rL363750: Add ReadCStringFromMemory for faster string reads (authored by aadsm, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm updated this revision to Diff 205211. aadsm added a comment. - Add a test to cover cross page reads - Make cache_line_size a static const variable inside the function. - Fix how we're passing name_buffer to ReadCStringFromMemory. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. LGTM, modulo the inline comments. Comment at: lldb/source/Host/common/NativeProcessProtocol.cpp:25-26 // NativeProcessProtocol Members +const size_t

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm updated this revision to Diff 204979. aadsm added a comment. - Address missing comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503 Files: lldb/include/lldb/Host/common/NativeProcessProtocol.h

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm updated this revision to Diff 204977. aadsm added a comment. Herald added a subscriber: emaste. - Make ReadCStringFromMemory return llvm::Expected> - Add documentation - Address other comments - Created `static const size_t g_cache_line_size` to avoid calling

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added inline comments. Comment at: lldb/unittests/Host/NativeProcessProtocolTest.cpp:128-133 + EXPECT_THAT_ERROR( + Process.ReadCStringFromMemory(0x0, [0], sizeof(string), bytes_read) + .ToError(), + llvm::Succeeded()); + EXPECT_STREQ(string, "hel");

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

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

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added a comment. In D62503#1540959 , @aadsm wrote: > Yeah, that's a good question and I did think about it at the time I wrote > D62715 . Here's my rationale: > Regarding the chunk reading in the ReadMemory I was

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm marked an inline comment as done. aadsm added a comment. Yeah, that's a good question and I did think about it at the time I wrote D62715 . Here's my rationale: Regarding the chunk reading in the ReadMemory I was not sure because I don't know how common

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added a comment. Given that you're improving the linux implementation (which is the only one that benefits from chunked reads) of ReadMemory in https://reviews.llvm.org/D62715, does it make sense to do the chunking here? After all, if an implementation (like NetBSD) has an efficient

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm updated this revision to Diff 203948. aadsm added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503 Files: lldb/include/lldb/Host/common/NativeProcessProtocol.h

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm updated this revision to Diff 203658. aadsm added a comment. Fix a little bug and add tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503 Files:

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm updated this revision to Diff 203084. aadsm added a comment. Make sure we always return a null terminated string Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503 Files:

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm updated this revision to Diff 202329. aadsm added a comment. Moved ReadCStringFromMemory to NativeProcessProtocol and address some other comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62503/new/ https://reviews.llvm.org/D62503

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added a comment. In D62503#1520435 , @aadsm wrote: > > However, now that I think about it, that is nonsense, because there is no > > way for us to say to the user that "we failed to read some initial bytes, > > but this is the memory contents

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm added a comment. > However, now that I think about it, that is nonsense, because there is no way > for us to say to the user that "we failed to read some initial bytes, but > this is the memory contents after that". So just using process_vm_readv, and > finishing up with ptrace sounds

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added a comment. In D62503#1519179 , @aadsm wrote: > Regarding changing the ReadMemory, yes, I was going to submit another patch > with it. I was going to do it in a different way though, where I would read > as much as possible with the

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm added a comment. Regarding changing the ReadMemory, yes, I was going to submit another patch with it. I was going to do it in a different way though, where I would read as much as possible with the process_vm_readv (fwiw, this function does not fail when it tries to read cross page, it

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

clayborg added a comment. In D62503#1518839 , @labath wrote: > It sounds like these improved reads would be beneficial for all kinds of > reads, and not just for reading c strings. Could this chunking logic be > implemented directly in ReadMemory? We

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

labath added a comment. It sounds like these improved reads would be beneficial for all kinds of reads, and not just for reading c strings. Could this chunking logic be implemented directly in ReadMemory? That would also allow you to test this by sending `m` which are deliberately chosen to

[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads

aadsm created this revision. aadsm added reviewers: clayborg, xiaobai. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. aadsm added a parent revision: D62502: Implement xfer:libraries-svr4:read packet. This is the fifth and final patch to improve module loading in a series