Skip to content

Source map fixes #6550

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 8 commits into from
May 2, 2024
Merged

Source map fixes #6550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1a79a7f
Keep debug locations at function start
vouillon Apr 23, 2024
9fc48ab
Do not clear the debug information when reaching the end of the sourc…
vouillon Apr 25, 2024
e045086
Propagate debug location from the function prolog to its first instru…
vouillon Apr 26, 2024
e08fe3b
Fix printing of epilogue location
vouillon Apr 26, 2024
7292806
Fix debug location smearing
vouillon Apr 26, 2024
63ddcfd
Update src/wasm/wasm-binary.cpp
vouillon Apr 30, 2024
4994c67
Improved comments
vouillon May 2, 2024
b68e90d
Stack IR
vouillon May 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
4 changes: 4 additions & 0 deletions src/passes/DebugLocationPropagation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ struct DebugLocationPropagation
if (auto it = locs.find(previous); it != locs.end()) {
locs[curr] = it->second;
}
} else if (self->getFunction()->prologLocation.size()) {
// Instructions may inherit their locations from the function
// prolog.
locs[curr] = *self->getFunction()->prologLocation.begin();
Copy link
Member

@kripken kripken Apr 29, 2024

Choose a reason for hiding this comment

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

How does this ensure it only affects the first instruction and not later ones?

Copy link
Contributor Author

@vouillon vouillon Apr 30, 2024

Choose a reason for hiding this comment

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

It should affect later instructions as well.
This piece of code:

 ;;@ a:1:1
 (func
    ;;@ a:1:1
    (nop)
    ;;@ a:1:1
    (nop)
 )

is printed as:

 ;;@ a:1:1
 (func
  (nop)
  (nop)
 )

So they should result in the same debug information when read back.

Copy link
Member

@kripken kripken Apr 30, 2024

Choose a reason for hiding this comment

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

Ah, you mean by the normal rules of propagating from the parent to children? So the first instruction inherits from the prolog, and children of the first instruction inherit from the first instruction, so effectively they inherit from the prologue too? If so that makes sense but the comment was confusing me. Perhaps The first instruction may inherit its location from the function prolog could be Instructions may inherit their locations from the function prolog. - ?

Copy link
Member

@kripken kripken Apr 30, 2024

Choose a reason for hiding this comment

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

Actually, on reading the code again, I think a simpler way to do this would be, instead of adding code here, to add code to getPrevious. That method could look at the function prologue if it does not find something else.

Copy link
Contributor Author

@vouillon vouillon May 2, 2024

Choose a reason for hiding this comment

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

I'm not sure what you mean since getPrevious returns a pointer to an expression. Do you mean I should move some of the code here to getPrevious so that it returns an optional location instead?

Copy link
Member

@kripken kripken May 2, 2024

Choose a reason for hiding this comment

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

Maybe I'm missing something, but right now getPrevious is

Expression* getPrevious() {
  if (expressionStack.empty()) {
    return nullptr;
  }
  assert(expressionStack.size() >= 1);
  return expressionStack[expressionStack.size() - 1];
}

I am suggesting it be something like

Expression* getPrevious() {
  if (expressionStack.empty()) {
    // Use the prologue, if there is one.
    if (self->getFunction()->prologLocation.size()) {
        return *self->getFunction()->prologLocation.begin();
    }
    return nullptr;
  }
  assert(expressionStack.size() >= 1);
  return expressionStack[expressionStack.size() - 1];
}

Then this function needs no changes, and can keep calling getPrevious() on line 63 and if it returns a non-null value, to use that.

Copy link
Contributor Author

@vouillon vouillon May 2, 2024

Choose a reason for hiding this comment

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

But prologLocation has type std::set<Function::DebugLocation>.

Copy link
Member

@kripken kripken May 2, 2024

Choose a reason for hiding this comment

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

Ah, right. Then more code would need to move into getPrevious, and then really it would need to be renamed. I agree it's not worth it, sorry for not thinking this totally through.

}
}
expressionStack.push_back(curr);
Expand Down
3 changes: 1 addition & 2 deletions src/passes/Print.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3025,8 +3025,7 @@ void PrintSExpression::visitDefinedFunction(Function* curr) {
// Print the stack IR.
printStackIR(curr->stackIR.get(), *this);
}
if (currFunction->epilogLocation.size() &&
lastPrintedLocation != *currFunction->epilogLocation.begin()) {
if (currFunction->epilogLocation.size()) {
// Print last debug location: mix of decIndent and printDebugLocation
// logic.
doIndent(o, indent);
Expand Down
1 change: 1 addition & 0 deletions src/wasm-binary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,7 @@ class WasmBinaryWriter {
void writeSourceMapProlog();
void writeSourceMapEpilog();
void writeDebugLocation(const Function::DebugLocation& loc);
void writeNoDebugLocation();
void writeDebugLocation(Expression* curr, Function* func);
void writeDebugLocationEnd(Expression* curr, Function* func);
void writeExtraDebugLocation(Expression* curr, Function* func, size_t id);
Expand Down
5 changes: 5 additions & 0 deletions src/wasm-stack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,8 +443,13 @@ class BinaryenIRToBinaryWriter
void emitDelegate(Try* curr) { writer.emitDelegate(curr); }
void emitScopeEnd(Expression* curr) { writer.emitScopeEnd(curr); }
void emitFunctionEnd() {
// Indicate the debug location corresponding to the end opcode
// that terminates the function code.
if (func->epilogLocation.size()) {
parent.writeDebugLocation(*func->epilogLocation.begin());
} else {
// The end opcode has no debug location.
parent.writeNoDebugLocation();
Copy link
Member

@kripken kripken Apr 29, 2024

Choose a reason for hiding this comment

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

Is this needed, since we prevent smearing across functions in another way anyhow? That is, this is the end of the function, so marking no-debug will not help any code but the next function, which is already handled by this PR elsewhere I think.

Copy link
Contributor Author

@vouillon vouillon Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is to prevent the debug location on the last instruction to smear on the function epilogue (that is, the end opcode at the end of the function code). So that this:

(func
   ;;@ a:1:1
   (nop)
   ;;@
)

does not get turned into this:

(func
   ;;@ a:1:1
   (nop)
   ;;@ a:1:1
)

Copy link
Member

@kripken kripken Apr 30, 2024

Choose a reason for hiding this comment

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

I see, thanks. Maybe add a comment saying that?

}
writer.emitFunctionEnd();
}
Expand Down
51 changes: 28 additions & 23 deletions src/wasm/wasm-binary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,8 +398,10 @@ void WasmBinaryWriter::writeFunctions() {
bool DWARF = Debug::hasDWARFSections(*getModule());
ModuleUtils::iterDefinedFunctions(*wasm, [&](Function* func) {
assert(binaryLocationTrackedExpressionsForFunc.empty());
size_t sourceMapLocationsSizeAtFunctionStart = sourceMapLocations.size();
BYN_TRACE("write one at" << o.size() << std::endl);
// Do not smear any debug location from the previous function.
writeNoDebugLocation();
size_t sourceMapLocationsSizeAtFunctionStart = sourceMapLocations.size();
size_t sizePos = writeU32LEBPlaceholder();
size_t start = o.size();
BYN_TRACE("writing" << func->name << std::endl);
Expand Down Expand Up @@ -1373,6 +1375,28 @@ void WasmBinaryWriter::writeDebugLocation(const Function::DebugLocation& loc) {
lastDebugLocation = loc;
}

void WasmBinaryWriter::writeNoDebugLocation() {
// Emit an indication that there is no debug location there (so that
// we do not get "smeared" with debug info from anything before or
// after us).
//
// We don't need to write repeated "no debug info" indications, as a
// single one is enough to make it clear that the debug information
// before us is valid no longer. We also don't need to write one if
// there is nothing before us.
if (!sourceMapLocations.empty() &&
sourceMapLocations.back().second != nullptr) {
sourceMapLocations.emplace_back(o.size(), nullptr);

// Initialize the state of debug info to indicate there is no current
// debug info relevant. This sets |lastDebugLocation| to a dummy value,
// so that later places with debug info can see that they differ from
// it (without this, if we had some debug info, then a nullptr for none,
// and then the same debug info, we could get confused).
initializeDebugInfo();
}
}

void WasmBinaryWriter::writeDebugLocation(Expression* curr, Function* func) {
if (sourceMap) {
auto& debugLocations = func->debugLocations;
Expand All @@ -1381,25 +1405,8 @@ void WasmBinaryWriter::writeDebugLocation(Expression* curr, Function* func) {
// There is debug information here, write it out.
writeDebugLocation(iter->second);
} else {
// This expression has no debug location. We need to emit an indication
// of that (so that we do not get "smeared" with debug info from anything
// before or after us).
//
// We don't need to write repeated "no debug info" indications, as a
// single one is enough to make it clear that the debug information before
// us is valid no longer. We also don't need to write one if there is
// nothing before us.
if (!sourceMapLocations.empty() &&
sourceMapLocations.back().second != nullptr) {
sourceMapLocations.emplace_back(o.size(), nullptr);

// Initialize the state of debug info to indicate there is no current
// debug info relevant. This sets |lastDebugLocation| to a dummy value,
// so that later places with debug info can see that they differ from
// it (without this, if we had some debug info, then a nullptr for none,
// and then the same debug info, we could get confused).
initializeDebugInfo();
}
// This expression has no debug location.
writeNoDebugLocation();
}
}
// If this is an instruction in a function, and if the original wasm had
Expand Down Expand Up @@ -2687,12 +2694,11 @@ void WasmBinaryReader::readFunctions() {

readVars();

std::swap(func->prologLocation, debugLocation);
func->prologLocation = debugLocation;
{
// process the function body
BYN_TRACE("processing function: " << i << std::endl);
nextLabel = 0;
debugLocation.clear();
willBeIgnored = false;
// process body
assert(breakStack.empty());
Expand Down Expand Up @@ -2931,7 +2937,6 @@ void WasmBinaryReader::readNextDebugLocation() {

if (nextDebugPos == 0) {
// We reached the end of the source map; nothing left to read.
debugLocation.clear();
return;
}

Expand Down
5 changes: 5 additions & 0 deletions src/wasm/wasm-stack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2839,8 +2839,13 @@ void StackIRToBinaryWriter::write() {
WASM_UNREACHABLE("unexpected op");
}
}
// Indicate the debug location corresponding to the end opcode that
// terminates the function code.
if (func->epilogLocation.size()) {
parent.writeDebugLocation(*func->epilogLocation.begin());
} else {
// The end opcode has no debug location.
parent.writeNoDebugLocation();
}
writer.emitFunctionEnd();
}
Expand Down
2 changes: 1 addition & 1 deletion test/binaryen.js/sourcemap.js.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ module.c
020: u r c e M a p p i n g U R L 0f m
030: o d u l e . w a s m . m a p

{"version":3,"sources":["module.c"],"names":[],"mappings":"wBAAE"}
{"version":3,"sources":["module.c"],"names":[],"mappings":"wBAAE,E"}
3 changes: 2 additions & 1 deletion test/fib-dbg.wasm.fromBinary
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,11 +209,12 @@
(br $label$4)
)
)
;;@ fib.c:8:0
(return
;;@ fib.c:8:0
(local.get $4)
)
)
;;@ fib.c:8:0
)
(func $runPostSets
(local $0 i32)
Expand Down
22 changes: 22 additions & 0 deletions test/lit/debug/source-map-smearing.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
;; RUN: wasm-opt %s -g -o %t.wasm -osm %t.wasm.map
;; RUN: echo >> %t.wasm.map
;; RUN: cat %t.wasm.map | filecheck %s

;; Also test with StackIR, which should have identical results.
;;
;; RUN: wasm-opt %s --generate-stack-ir -o %t.wasm -osm %t.map -g -q
;; RUN: echo >> %t.wasm.map
;; RUN: cat %t.wasm.map | filecheck %s

;; Check that the debug locations do not smear beyond a function
;; epilogue to the next function. The encoded segment 'C' means that
;; the previous segment is indeed one-byte long.
;; CHECK: {"version":3,"sources":["foo"],"names":[],"mappings":"yBAAC,C,GACC"}
(module
(func $0
;;@ foo:1:1
)
(func $1
;;@ foo:2:2
)
)
5 changes: 5 additions & 0 deletions test/lit/debug/source-map-stop.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
;; RUN: wasm-opt %s -g -o %t.wasm -osm %t.wasm.map
;; RUN: wasm-opt %t.wasm -ism %t.wasm.map -S -o - | filecheck %s

;; Also test with StackIR, which should have identical results.
;;
;; RUN: wasm-opt %s --generate-stack-ir -o %t.wasm -osm %t.map -g -q
;; RUN: wasm-opt %t.wasm -ism %t.map -q -o - -S | filecheck %s

;; Verify that writing to a source map and reading it back does not "smear"
;; debug info across adjacent instructions. The debug info in the output should
;; be identical to the input.
Expand Down