Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

i#7046 Add Linux support to dr_create_memory_dump. #7047

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

i#7046 Add Linux support to dr_create_memory_dump. #7047

wants to merge 2 commits into from

Conversation

ivankyluk
Copy link
Contributor

@ivankyluk ivankyluk commented Oct 18, 2024 •
edited
Loading

This is the first change to add Linux support to dr_create_memory_dump. The memory dump is written in ELF format and save under the logs directory following the WIN example.

The code is test with the following change:

   dr_memory_dump_spec_t spec;
   spec.size = sizeof(dr_memory_dump_spec_t);
   spec.flags = DR_MEMORY_DUMP_ELF;
   dr_create_memory_dump(&spec);

and use "readelf -a" to ensure the elf file is built correctly.

Issue: #7046

@ivankyluk ivankyluk force-pushed the i7046-add-linux-support-to-dr_create_memory_dump branch from a6cee2c to f37ddc3 Compare October 18, 2024 21:04
@ivankyluk ivankyluk changed the title Add Linux (x64 only) support to dr_create_memory_dump. i#7046 Add Linux (x64 only) support to dr_create_memory_dump. Oct 18, 2024
@ivankyluk ivankyluk force-pushed the i7046-add-linux-support-to-dr_create_memory_dump branch 2 times, most recently from 1c8c659 to 971b2d1 Compare October 18, 2024 21:46
@ivankyluk ivankyluk changed the title i#7046 Add Linux (x64 only) support to dr_create_memory_dump. i#7046 Add Linux support to dr_create_memory_dump. Oct 18, 2024
@ivankyluk ivankyluk force-pushed the i7046-add-linux-support-to-dr_create_memory_dump branch 5 times, most recently from 39dcddf to 05727e4 Compare October 18, 2024 23:29
@ivankyluk ivankyluk self-assigned this Oct 19, 2024
@ivankyluk ivankyluk force-pushed the i7046-add-linux-support-to-dr_create_memory_dump branch from 05727e4 to 37f8abf Compare October 19, 2024 01:41
@derekbruening
Copy link
Contributor

Sorry for the delay: I missed this. Please ping earlier in the future.

@derekbruening
Copy link
Contributor

@ivankyluk ivankyluk force-pushed the i7046-add-linux-support-to-dr_create_memory_dump branch from a6cee2c to f37ddc3
4 days ago

Please don't force-push on shared branches (documented at https://dynamorio.org/page_code_reviews.html#autotoc_md118 and in other places)

@ivankyluk ivankyluk marked this pull request as draft October 22, 2024 22:50
@ivankyluk
Copy link
Contributor Author

I've pull a new request #7053.

derekbruening
derekbruening reviewed Oct 22, 2024
View reviewed changes
core/CMakeLists.txt
@@ -390,6 +390,7 @@ if (UNIX)
set(OS_SRCS ${OS_SRCS} unix/loader_android.c)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

save under the logs directory following the WIN example.

grammar: s/save/saved/

core/CMakeLists.txt
@@ -390,6 +390,7 @@ if (UNIX)
set(OS_SRCS ${OS_SRCS} unix/loader_android.c)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is test with the following change:

Grammar: s/test/tested/

core/lib/dr_tools.h
/**
* Memory dump in Executable and Linkable Format.
*
* \note Linux and X64 only.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"X64 Linux only" would be more clear (as is, it could mean Linux is supported and X64 (Windows, Mac, etc.) is supported).

core/lib/dr_tools.h
/**
* Memory dump in Executable and Linkable Format.
*
* \note Linux and X64 only.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a release note to api/docs/release.dox on the new feature.

core/unix/coredump.c

#ifdef X64
# define ELF_HEADER_TYPE Elf64_Ehdr
# define ELF_PROGRAM_HEADER_TYPE Elf64_Phdr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all in module_elf.h? Can we share from there, or in a new header?

core/unix/coredump.c
}

#ifdef DEADLOCK_AVOIDANCE
/* first turn off deadlock avoidance for this thread (needed for live dump
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Capitalize and punctuate new comments.

core/unix/coredump.c
dcontext->thread_owned_locks = NULL;
}
#endif
/* only allow one thread to dumpcore at a time, also protects static
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Capitalize and punctuate new comments.

core/unix/coredump.c
d_r_mutex_unlock(&dump_core_lock);

#ifdef DEADLOCK_AVOIDANCE
/* restore deadlock avoidance for this thread */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Capitalize and punctuate new comments.

core/unix/coredump.c

#ifdef DEADLOCK_AVOIDANCE
/* first turn off deadlock avoidance for this thread (needed for live dump
* to try to grab all_threads and thread_initexit locks) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't see code grabbing those locks?

core/utils.c
@@ -417,7 +417,8 @@ locks_not_closed()
cur_lock->rank ==
LOCK_RANK(options_lock)
/* This lock can be used parallel to detach cleanup. */
IF_UNIX(|| cur_lock->rank == LOCK_RANK(detached_sigact_lock)))) {
IF_UNIX(|| cur_lock->rank == LOCK_RANK(detached_sigact_lock))
IF_LINUX(|| cur_lock->rank == LOCK_RANK(dump_core_lock)))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new lock should be cleaned up: please add a destroy instead of an exception.

@derekbruening
Copy link
Contributor

I've pull a new request #7053.

Is that b/c of the force pushes? Probably not necessary; if anyone had pulled the branch (not likely) the damage is already done; avoiding future ones will avoid review issues now that reviewing has started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekbruening derekbruening derekbruening left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@ivankyluk ivankyluk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivankyluk @derekbruening