Source map fixes

[vouillon]

Some debug locations could be dropped at the beginning and end of functions:

• if a source map segment starts before the beginning of the code, it was not taken into account except for the function prologue;
• the last segment of the source map was ignored after one instruction;
• for coherence with the text printer, the text parser should propagate the debug location of the function prologue to its first instruction.
• the text printer should always print the debug location of the function epilogue if there is one (for coherence, since one repeats a debug location at the same indentation level for readability, and for round-tripping since the test parser does not propagate debug locations to the function epilogue).

Also, the binary writer was smearing the debug location of the last instruction to the function epilogue, and the debug location of a previous function could smear to the prologue of the next function.

[kripken]

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

[vouillon]

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.

[kripken]

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. - ?

[kripken]

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.

[vouillon]

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?

[kripken]

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.

[vouillon]

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

[kripken]

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.

[kripken]

Aside from the comments this looks right to me. I also verified it does not break any debug/source-map tests in Emscripten, which gives additional confidence.

[kripken]

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.

[vouillon]

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
)

[kripken]

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

[kripken]

Before we land this, can you please merge in latest main, just to make sure there is no test conflict with other recent updates?

[vouillon]

Before we land this, can you please merge in latest main, just to make sure there is no test conflict with other recent updates?

This is done already. I only had to make a small change to the Stack IR source map support (see last commit).

[kripken]

Thanks, I see, I missed that this was forced pushed and I was looking for a merge commit...