[lldb] [Process/FreeBSDKernel] List threads in correct order#178306
[lldb] [Process/FreeBSDKernel] List threads in correct order#178306DavidSpickett merged 1 commit intollvm:mainfrom
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: Minsoo Choo (mchoo7) ChangesIn FreeBSD, allproc is a prepend list and new processes are appeneded at head. This results in reverse pid order, so we first need to order pid incrementally then print threads according to the correct order. Before: After: Full diff: https://github.com/llvm/llvm-project/pull/178306.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
index d209e5b5384f3..43a875f5c541e 100644
--- a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
@@ -183,13 +183,21 @@ bool ProcessFreeBSDKernel::DoUpdateThreadList(ThreadList &old_thread_list,
// from FreeBSD sys/param.h
constexpr size_t fbsd_maxcomlen = 19;
- // iterate through a linked list of all processes
- // allproc is a pointer to the first list element, p_list field
- // (found at offset_p_list) specifies the next element
+ // pid is in reverse order. Order it incrementally
+ std::vector<lldb::addr_t> process_addrs;
for (lldb::addr_t proc =
ReadPointerFromMemory(FindSymbol("allproc"), error);
proc != 0 && proc != LLDB_INVALID_ADDRESS;
proc = ReadPointerFromMemory(proc + offset_p_list, error)) {
+ process_addrs.push_back(proc);
+ }
+
+ // iterate through a linked list of all processes
+ // allproc is a pointer to the first list element, p_list field
+ // (found at offset_p_list) specifies the next element
+ for (auto proc_it = process_addrs.rbegin(); proc_it != process_addrs.rend();
+ ++proc_it) {
+ lldb::addr_t proc = *proc_it;
int32_t pid =
ReadSignedIntegerFromMemory(proc + offset_p_pid, 4, -1, error);
// process' command-line string
|
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
I agree that it's more logical to list them in ascending order. Code is fine, just some comments on the comments.
As with the other PR, please get the existing tests running and see what the results are. I think they either do check this detail, or should, and should be extended to do so.
lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
Outdated
Show resolved
Hide resolved
JDevlieghere
left a comment
JDevlieghere
left a comment
There was a problem hiding this comment.
A few more nits since you're touching these lines :-)
lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
Outdated
Show resolved
Hide resolved
lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
Outdated
Show resolved
Hide resolved
In FreeBSD, allproc is a prepend list and new processes are appeneded at head. This results in reverse pid order, so we first need to order pid incrementally then print threads according to the correct order. Signed-off-by: Minsoo Choo <minsoochoo0122@proton.me>
|
Please use fixup instead of force push. See: https://llvm.org/docs/GitHub.html#updating-pull-requests |
Sorry... I already lost previous commits (I run |
|
On testing: we discussed this on another PR and the situation is that because the coredumps are so large, they were generated by patching LLDB to make them smaller. The minidump format tests don't need that, I don't think. They are also only run when you've got specific libraries installed, which none of our usual CI machines do. So if the results drift, it's not going to impact anyone but FreeBSD users. So I personally am ok with @mchoo7 doing these changes and then updating the tests later, and maybe we can figure out a better way for re-generating that doesn't involve patching lldb. I hope others agree with that. |
DavidSpickett
left a comment
DavidSpickett
left a comment
There was a problem hiding this comment.
LGTM assuming no one objects to the test plan, or lack of, for the time being.
|
ping @emaste |
Yeah, this situation is a bit unfortunate, but I agree this patch improves LLDB on FreeBSD and shouldn't affect the test results (as run in and reported by CI). Hopefully we (FreeBSD LLVM community) can work on improving CI. |
|
@mchoo7 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
7 participants
In FreeBSD, allproc is a prepend list and new processes are appeneded at head. This results in reverse pid order, so we first need to order pid incrementally then print threads according to the correct order.
Before:
After: