Try to improve C debugging experience #2134

bugadani · 2024-02-03T00:25:10Z

We need to set frame_section.set_address_size as gimli defaults to host (8) which is just incorrect
This PR propagates (some?) CFA information to the evaluation as required by my elf
This PR moves CFA calculation to before determining frame base. In my case, the reverse failed due to CFA being None.

With this, I now have a decent call stack, but variable values are still not filly implemented (enums!). Also, struct fields are always read with the same byte count as the struct itself, it looks like.

bugadani · 2024-02-03T00:27:48Z

probe-rs/src/debug/debug_info.rs

@@ -95,6 +96,8 @@ impl DebugInfo {

        while let Ok(Some(header)) = iter.next() {
            if let Ok(unit) = dwarf_cow.unit(header) {
+                // TODO: maybe it's not correct to read from arbitrary units


This is probably just a coincidence and not a general truth. Still, this was the easiest 4-byte pointer size I found nearby.

bugadani · 2024-02-04T14:16:22Z

Two changes stand out in the failing test:

 8279       │-- function_name: "<unknown function @ 0x0000013c>"
       8288 │+- function_name: "<unknown function @ 0x0000013c> : ERROR: UNWIND: Tried to unwind `RegisterRule` at CFA = None."

Not sure why, is this PR is passing CFA to more places, is this exposing some other issue, or is the PR incorrect somewhere?

 8571       │-      value:
 8572       │-        U32: 536887296
       8581 │+      value: ~

I have no idea what happened here.

noppej

@bugadani Thank you for this. I have the source code for the test cases setup, so will be debugging those test cases to understand the changes, and let you know. Chances are they are easily resolved, but the unwind is hard to fix once it is broken, so I will temporarily (1-2 days) block this merge, until we can understand the root cause of those changes.

Yatekii · 2024-02-04T20:28:51Z

I have the source code for the test cases setup

Can you publish the source code? :)

noppej · 2024-02-04T21:14:40Z

Can you publish the source code? :)

Remember these ... https://github.com/probe-rs/probe-rs-debugger-test/blob/master/README.md ;-)

noppej · 2024-02-05T14:46:37Z

Two changes stand out in the failing test:

@bugadani I've reviewed these changes and you can go ahead and 'accept' them with cargo insta review.

Also, is there a reason why this is still in DRAFT?

bugadani · 2024-02-05T14:49:56Z

Also, is there a reason why this is still in DRAFT?

yes, I'm not ready :)

noppej

@bugadani I know this is in draft status, but I'm approving the changes you've made so far, in case you want to think about doing further work in a new PR.

Thank you for moving forward the functionality for C binaries, and also, thank you for taking the extra time to do code cleanup when you see opportunities to do so. Your contributions are very valuable!

noppej · 2024-02-05T14:48:24Z

probe-rs/src/debug/stack_frame.rs

    /// - Complex variables and pointers will have additional children.
    ///   - This structure is recursive until a base type is encountered.
    pub local_variables: Option<VariableCache>,
+    /// The value of the stack pointer just before the CALL instruction in the parent function.
+    pub canonical_frame_address: Option<u64>,


I'm curious ... you instroduced a new struct for StackFrameInfo, and all the contents is duplicated in StackFrame ... should we just have a StackFrameInfo inside StackFrame?

There's a difference: StackFrameInfo just borrows the registers from StackFrame and does not own them. At first I wanted to pass StackFrame around but it wasn't available everywhere, though maybe that has changed now?

noppej · 2024-02-05T14:50:49Z

probe-rs/src/debug/variable.rs

@@ -646,6 +642,36 @@ impl Variable {
                        |value| VariableValue::Valid(value.to_string()),
                    ),
                    "None" => VariableValue::Valid("None".to_string()),
+
+                    // C types - should probably branch on the language


We would have to pretty confident that it is only types that are exclusive C, and not 'overlapped' in other languages. Ideally if we find types with different names, but same underlying storage, then we should not duplicate the code required to resolve (and/or udpdate) values.

char is painful, I want to branch on the language here.

noppej · 2024-02-05T14:59:27Z

probe-rs/src/debug/unit_info.rs

    ) -> Result<ExpressionResult, DebugError> {
+        trait ResultExt {


Nice little mechanism to reduce code duplication later on ... after this, I won't go to bed stupid tonight. Thanks :-)

noppej · 2024-02-05T15:06:58Z

probe-rs/src/debug/debug_info.rs

-                continue;
+            if !function_dies.is_empty() {
+                functions = Some((unit_info, function_dies));
+                break;


Can you help me understand why you changed the continue to break?

I also changed the condition from is_empty to !is_empty. I split the original loop into a "find" loop and the code afterwards that handles the found data.

bugadani · 2024-02-05T17:06:57Z

I read that you're find with the snapshot changes, but do you know their root cause, too? I would prefer not to introduce an issue if we already know its cause.

I've marked the PR ready in case you want to merge it in this state. I'll follow up with more work, there are two outstanding issues right now:

struct fields seem to be read with wrong byte counts
enums aren't evaluated
the above question

noppej · 2024-02-05T17:17:40Z

struct fields seem to be read with wrong byte counts

enums aren't evaluated

The tests (and snapshots) have extensive struct fields and enums, and they unwind correctly. Are you bumping into this issue when you test locally? If so, I think we should fix that in a separate PR. If not, can you give an example?

the above question

Those belong to stack frames that are incorrectly unwound as it is. We should either stop unwinding, or preferably (#1667) fix the way we unwind past the main trampoline.

bugadani · 2024-02-05T17:19:10Z

If not, can you give an example?

any of my C enums :) I specifically have issues with C, not with Rust code, so I think their debug info are just encoded differently.

noppej · 2024-02-05T17:19:16Z

I've marked the PR ready in case you want to merge it in this state. I'll follow up with more work

Even if this doesn't achieve end result, it does already change the api (for StackFrame). Would you mind adding a small CHANGELOG?

noppej · 2024-02-05T17:26:08Z

If not, can you give an example?

any of my C enums :) I specifically have issues with C, not with Rust code, so I think their debug info are just encoded differently.

Ouch!

noppej · 2024-02-05T17:37:32Z

Awesome. I'm gonna merge so that we can do additional work in new PR's ... it will make it easier to review :)

bugadani commented Feb 3, 2024

View reviewed changes

noppej self-requested a review February 4, 2024 18:45

noppej requested changes Feb 4, 2024

View reviewed changes

noppej approved these changes Feb 5, 2024

View reviewed changes

noppej mentioned this pull request Feb 5, 2024

Tracking issue for unwind testing in the debug/debuginfo.rs module. #1916

Open

10 tasks

bugadani marked this pull request as ready for review February 5, 2024 17:05

bugadani added 15 commits February 5, 2024 18:23

Log incomplete result reasons, fix typo

f36faf3

Stora CFA in StackFrame

468cde2

Pass CFA to evaluation

8c6192b

Group stackframe related info

0c04d5c

Outdent some code

3d69540

Simplify path

11f01fa

Set target-specific address size

bd9835d

Correct comment

3325405

Extract incomplete -> unavailable conversion

3aa7cdc

Pass StackFrameInfo to frame_base

294a612

Only create unknown function name if needed

00a04b1

Extract function

5505f6c

Extract error cases into fn

d309596

Don't require Box in get_unwind_info

dfd64b2

Separate loop from function processing

1a15ce5

bugadani added 7 commits February 5, 2024 18:23

Determine CFA before frame base

afec339

Pass CFA to unwind_register

7c57c48

Avoid importing enum variants

d061760

Clean up

dd8b916

Read C scalars

fa9b28a

Bless snapshots

aaf345b

Add changelog entry

b9cdd68

noppej added this pull request to the merge queue Feb 5, 2024

Merged via the queue into probe-rs:master with commit e75bff6 Feb 5, 2024
18 checks passed

bugadani deleted the cfa branch February 5, 2024 17:43

bugadani mentioned this pull request Feb 8, 2024

Extract language specific variable type handling #2150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to improve C debugging experience #2134

Try to improve C debugging experience #2134

bugadani commented Feb 3, 2024 •

edited

Loading

bugadani Feb 3, 2024

bugadani commented Feb 4, 2024 •

edited

Loading

noppej left a comment

Yatekii commented Feb 4, 2024

noppej commented Feb 4, 2024 •

edited

Loading

noppej commented Feb 5, 2024

bugadani commented Feb 5, 2024

noppej left a comment

noppej Feb 5, 2024

bugadani Feb 5, 2024

noppej Feb 5, 2024

bugadani Feb 5, 2024

noppej Feb 5, 2024

noppej Feb 5, 2024

bugadani Feb 5, 2024

bugadani commented Feb 5, 2024

noppej commented Feb 5, 2024

bugadani commented Feb 5, 2024

noppej commented Feb 5, 2024

noppej commented Feb 5, 2024

noppej commented Feb 5, 2024

		) -> Result<ExpressionResult, DebugError> {
		trait ResultExt {

Try to improve C debugging experience #2134

Try to improve C debugging experience #2134

Conversation

bugadani commented Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

bugadani commented Feb 4, 2024 • edited Loading

noppej left a comment

Choose a reason for hiding this comment

Yatekii commented Feb 4, 2024

noppej commented Feb 4, 2024 • edited Loading

noppej commented Feb 5, 2024

bugadani commented Feb 5, 2024

noppej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bugadani commented Feb 5, 2024

noppej commented Feb 5, 2024

bugadani commented Feb 5, 2024

noppej commented Feb 5, 2024

noppej commented Feb 5, 2024

noppej commented Feb 5, 2024

bugadani commented Feb 3, 2024 •

edited

Loading

bugadani commented Feb 4, 2024 •

edited

Loading

noppej commented Feb 4, 2024 •

edited

Loading