Skip to content

[DebugInfo] Add debug info to the values emitted in GlobalStructInference #6709

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

Merged
kripken merged 5 commits into from
Jul 2, 2024
Merged

[DebugInfo] Add debug info to the values emitted in GlobalStructInference #6709

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef617be
prep
kripken Jul 2, 2024
d821a6e
missing
kripken Jul 2, 2024
ec1a0ab
undo
kripken Jul 2, 2024
7a192d2
test
kripken Jul 2, 2024
ebbff7a
show the fix
kripken Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
show the fix
  • Loading branch information
@kripken
kripken committed Jul 2, 2024
commit ebbff7aee4bb4e18be731e4dda5bcc00f9486479
2 changes: 1 addition & 1 deletion src/passes/GlobalStructInference.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ struct GlobalStructInference : public Pass {

// This value replaces the struct.get, so it should have the same
// source location.
// debuginfo::copyOriginalToReplacement(curr, ret, getFunction());
debuginfo::copyOriginalToReplacement(curr, ret, getFunction());

return ret;
};
Expand Down
4 changes: 2 additions & 2 deletions test/lit/passes/gsi-debug.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@
;; CHECK-NEXT: (drop
;; CHECK-NEXT: ;;@ struct.c:20:2
Copy link
Member

@aheejin aheejin Jul 2, 2024

Choose a reason for hiding this comment

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

Why did the select have debug info even before this PR (and not the children)?

Copy link
Member Author

@kripken kripken Jul 2, 2024

Choose a reason for hiding this comment

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

The select received it because replaceCurrent copies debug info, and we replace the struct.get with the select. But it doesn't copy recursively into children (we've considered that before, but it's not clear if it's correct, and would be slower).

Copy link
Member

@aheejin aheejin Jul 2, 2024

Choose a reason for hiding this comment

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

I thought we were doing that..?

binaryen/src/wasm-traversal.h

Lines 125 to 129 in cdf8139

// Copy debug info, if present.
if (currFunction) {
debuginfo::copyOriginalToReplacement(
getCurrent(), expression, currFunction);
}

Copy link
Member Author

@kripken kripken Jul 2, 2024

Choose a reason for hiding this comment

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

Yes, but not recursively, just on the replacement:

binaryen/src/ir/debuginfo.h

Line 43 in cdf8139

debugLocations[replacement] = iter->second;

So the select gets it, but not the select's children.

;; CHECK-NEXT: (select
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: ;;@ struct.c:20:2
;; CHECK-NEXT: (i32.const 42) (; i32 ;)
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: ;;@ struct.c:20:2
;; CHECK-NEXT: (i32.const 1337) (; i32 ;)
;; CHECK-NEXT: ;;@
;; CHECK-NEXT: (ref.eq
Expand Down