Skip to content

Add a function to print object info from the space for debugging #1269

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.

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

Merged
merged 7 commits into from
May 14, 2025

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Feb 4, 2025

This PR adds a non-mangled function to print object information for debugging. I also added a section in the doc about the function.

@qinsoon qinsoon marked this pull request as ready for review May 12, 2025 03:56
@qinsoon qinsoon requested a review from wks May 12, 2025 03:56
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

I have some high-level suggestions.

  • The argument of mmtk_debug_print_object_info should be an Address instead of ObjectReference because GDB and rr users may pass any number to that function.
  • Because the object may not be valid, we need to check if the side metadata are mapped before we attempt to read any side metadata.
  • Even with all the checking in place, it is still better to print one piece of information at a time using multiple println! statements. If any step panics (or cause SIGSEGV if it reads unmapped metadata), the user can still see partial information.

src/mmtk.rs Outdated
Comment on lines 603 to 608
println!(
"{}",
SFT_MAP
.get_checked(object.to_raw_address())
.debug_get_object_info(object)
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of printing the result here, I suggest we let debug_get_object_info gradually print more and more information. The reason is that when the user uses GDB or rr to debug a runtime, chance is high that the heap may have already been corrupted, and the metadata may have been inconsistent. In this case, one of the steps inside debug_get_object_info may go wrong and panic before it returns a human-readable string. Instead, if we let it gradually print more and more info, it can make the best effort even if it fails to finish normally

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Now each function impl only fetches one piece of metadata at a time, and print one line at a time.

{
ret += &format!(
"vo bit = {}",
crate::util::metadata::vo_bit::is_vo_bit_set(_object)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is_vo_bit_set doesn't check if the metadata is mapped or not. We should call is_mmtk_object or equivalently is_vo_bit_set_for_addr instead. And we should actually check it in crate::mmtk::mmtk_debug_print_object_info when converting the Address to ObjectReference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a check to check if the object is mapped or not before forwarding to SFT. If the object is mapped, we can assume all the metadata can be accessed.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the object is mapped, but some side metadata is not mapped, then it is a bug in MMTk core. We do not expect the function to work properly, and we do not expect the function can help us figure out the bug either.

@qinsoon
Copy link
Member Author

qinsoon commented May 13, 2025

The argument of mmtk_debug_print_object_info should be an Address instead of ObjectReference because GDB and rr users may pass any number to that function.

I don't agree with this. We do want the user to give us an object reference (even if it is represented as a usize in the debugger). The intention of this function is to print information for an object -- we don't want to access object metadata for an arbitrary address.

We can do our best guess in the function to rule out some possible issues, such as unmapped address/metadata, etc. But the function may crash if the heap/metadata is corrupted, or if the argument is invalid. After all this is just a utility function to help debugging.

@wks
Copy link
Collaborator

wks commented May 13, 2025

The argument of mmtk_debug_print_object_info should be an Address instead of ObjectReference because GDB and rr users may pass any number to that function.

I don't agree with this. We do want the user to give us an object reference (even if it is represented as a usize in the debugger). The intention of this function is to print information for an object -- we don't want to access object metadata for an arbitrary address.

We can do our best guess in the function to rule out some possible issues, such as unmapped address/metadata, etc. But the function may crash if the heap/metadata is corrupted, or if the argument is invalid. After all this is just a utility function to help debugging.

I mean, the user SHOULD always pass an object reference. But the user usually cannot verify if the address is valid. They usually copy an address from a log message or a stack frame info, such as

#3 0x00007ffff7e77090 in some_function(obj=1073741828) from /xxxx/xxx.so

The user probably don't use a calculator to check if 1073741828 is a multiple of 8 (in fact it is not) before invoking p mmtk_debug_print_object_info(1073741828). That's why the parameter type is Address. mmtk_debug_print_object_info will verify it, and make it an ObjectReference. If the parameter type is already ObjectReference, it will silently accept 1073741828 as the value of a ObjectReference and pass it to other places that assume it is aligned, and some weird error will occur.

@qinsoon
Copy link
Member Author

qinsoon commented May 13, 2025

If the user passes an object reference, we print information for the object. If they don't pass an object reference, they get what they get, which may not make any sense at all.

Without VO bit, we cannot tell if an address is an object reference or not. We can make guesses and rule out some least possible cases, but we cannot tell definitely that the address is an object or not. In such a case, we can't access the object metadata in a meaningful way if we are not sure if the address is an object reference.

I think we can just require an object reference. It is the user's responsibility to pass us a valid object reference. However, we do some checks in case that the reference is obviously invalid.

src/mmtk.rs Outdated
Comment on lines 606 to 610
assert!(
object.to_raw_address().is_mapped(),
"{} is not mapped in MMTk",
object
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an assertion. If this fails, it will panic. It is right for normal execution, but this function is for calling in GDB. For example,

(gdb) p (int)abort()

Program received signal SIGABRT, Aborted.
0x00007ffff7e0174c in ?? () from /usr/lib/libc.so.6
The program being debugged was signaled while in a function called from GDB.
GDB remains in the frame where the signal was received.
To change this behavior use "set unwind-on-signal on".
Evaluation of the expression containing the function
(abort) will be abandoned.
When the function is done executing, GDB will silently stop.
(gdb) 

It is better to just use println! to print an error message, and return if it is impossible to proceed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 105 to 107
println!("In {}", self.name());
object_forwarding::debug_print_object_forwarding_info::<VM>(object);
crate::policy::sft::debug_print_object_global_info(object);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Every implementation of debug_print_object_info starts with println!("In {}", self.name()); and ends with debug_print_object_global_info. We can hoist both of them into the caller (i.e. mmtk_debug_print_object_info).

Copy link
Member Author

Choose a reason for hiding this comment

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

I put println!("In {}", self.name()); to the caller. I realized that we cannot put debug_print_object_global_info to the caller, as we need the VM parameter which we only have for each space. I moved the function to CommonSpace, and adds printing for log bits there.

@qinsoon
Copy link
Member Author

qinsoon commented May 14, 2025

The above issues are fixed.

Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon qinsoon enabled auto-merge May 14, 2025 02:59
@qinsoon qinsoon added this pull request to the merge queue May 14, 2025
Merged via the queue into mmtk:master with commit 0790db6 May 14, 2025
32 checks passed
@qinsoon qinsoon deleted the debug-print-object-info branch May 14, 2025 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants