Source maps: allow to specify that an expression has no debug info in text parsers #6520
Conversation
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Isn't 0,0,0 a valid debug location? I think it would be better to use an empty std::optional to signify the absence of a debug location.
vouillon
force-pushed
the
absent-locs
branch
4 times, most recently
from
9ddf666 to
909c3df
Compare
src/wasm-ir-builder.h
Outdated
| Function* func; | ||
| Builder builder; | ||
| std::optional<Function::DebugLocation> debugLoc; | ||
| std::optional<std::optional<Function::DebugLocation>> debugLoc; |
There was a problem hiding this comment.
Worth a comment here too I think.
There was a problem hiding this comment.
Reading the comment, it's not immediately obvious how those three modes are represented in the data structure. Not hard to figure out but it might be better to be explicit, that is, "we explicitly specified that this instruction has no debug location" could be followed by "(the internal optional is nullopt)".
But reading this another time now, maybe we can do better than nested optionals. How about
// The location lacks debug info as it was marked as not having it.
struct NoDebug : public std::monostate {};
// The location lacks debug info, but was not marked as not having it, and it
// can receive it from the parent (if the parent has it).
struct CanReceiveDebug : public std::monostate {};
using DebugVariant = std::variant<NoDebug, CanReceiveDebug, Function::DebugLocation>;See e.g. src/ir/possible-contents.h for example code that uses that variant pattern.
| if (debugLocEnd == debugLoc) { | ||
| loc = nullptr; | ||
| return; | ||
| } |
There was a problem hiding this comment.
How does this relate to the PR - is it necessary because of something, or necessary for a new test, or something else?
There was a problem hiding this comment.
This is just parsing ;;@ with nothing after.
test/fib-dbg.wasm.fromBinary
Outdated
| (br $label$4) | ||
| ) | ||
| ) | ||
| ;;@ |
There was a problem hiding this comment.
I tried to look for this in the binary. What I did is add a printf here:
binaryen/src/wasm/wasm-binary.cpp
Line 2965 in f44dcd4
I believe that is the right place for reading a source map entry that says "this has no debug info, explicitly". But I don't see it reached. Am I doing something wrong?
There was a problem hiding this comment.
No, you are right. One should probably not clear the debug location here:
binaryen/src/wasm/wasm-binary.cpp
Line 2933 in f44dcd4
|
I have opened a separate PR #6550 that include the source map fixes. I'll rebase this PR once the other is merged. |
vouillon
force-pushed
the
absent-locs
branch
7 times, most recently
from
f91c476 to
6d744c4
Compare
… text parsers The comment `;;@` with nothing else can be used to specify that the following expression does not have any debug info associated to it. This can be used to stop the automatic propagation of debug info in the text parsers. The text printer has also been updated to output this comment when needed.
This is not supported anyway, since this relies on the indentation information, which is not updated when minifying.
|
Is this ready for review after the other PR landed? |
|
Yes, this is ready |
| ;; CHECK-NEXT: ;;@ | ||
| ;; CHECK-NEXT: (i32.add | ||
| ;; CHECK-NEXT: (local.get $x) | ||
| ;; CHECK-NEXT: (local.get $y) |
There was a problem hiding this comment.
To make sure I understand, these local.get have no debug annotation, like their parent, and so we did not print anything for them? That is, "nothing" propagates to children just like an actual debug location does?
There was a problem hiding this comment.
This might be worth clarifying in the README.
There was a problem hiding this comment.
Right. I have updated the README.
3 participants
The comment
;;@with nothing else can be used to specify that the following expression does not have any debug info associated to it. This can be used to stop the automatic propagation of debug info in the text parsers.The text printer has also been updated to output this comment when needed.