Issue 2686

[JesseCodeBones]

Fixes #2686

Changes proposed in this pull request:

• optimize data struct of range and expressRef, range 1 --> n expressionRef.
• add capacity for complier test to verify the sourceMap output

• I've read the contributing guidelines
• I've added my name and email to the NOTICE file

[dcodeIO]

Having one map per range seems excessive, memory-wise. Am I assuming correctly that the underlying issue was that the same debug info reference cannot be reused on multiple output expressions, hence there must be multiple copies? If so, is there perhaps a better way to make such copies?

[JesseCodeBones]

Having one map per range seems excessive, memory-wise. Am I assuming correctly that the underlying issue was that the same debug info reference cannot be reused on multiple output expressions, hence there must be multiple copies? If so, is there perhaps a better way to make such copies?

yeah, logically same range can generate multi expressionRef, from the trace we found it can happened that debugLocations contains the expression reference that donot belong to current function, we found this issue with template function.

** debug info insert trace ** ->

targetFunctionName=assembly/index/testFoo<assembly/index/AA|null>, debugFunctionName=assembly/index/testFoo<assembly/index/AA|null>, expressionRef=317368
targetFunctionName=assembly/index/testFoo<assembly/index/AA|null>, debugFunctionName=assembly/index/testFoo<assembly/index/AA>, expressionRef=496956
targetFunctionName=assembly/index/testFoo<assembly/index/AA|null>, debugFunctionName=assembly/index/testFoo<assembly/index/AA|null>, expressionRef=317384
targetFunctionName=assembly/index/testFoo<assembly/index/AA|null>, debugFunctionName=assembly/index/testFoo<assembly/index/AA>, expressionRef=496968

I suppose it is better to correct the data structure of range and expressionRef.
solutions could be:

1. create new structure for debugLocation like: {range, functionRef, expressionRef}
2. current solution, range debug info should be a map {functionRef, expressionRef}
   for the memory usage issue, normally debugLocation belongs to Function class, so the data has been splited with function level, I think it is acceptable.
   WDYT

[dcodeIO]

Hmm, what if we'd instead clone the range when calling addDebugLocation, so debugLocations is unique for each function, with each range having its own debugInfoRef? Or perhaps remove the caching entirely, and add debugInfoRef on demand? Haven't looked into this in detail yet, so I might be missing something, it's just that additional maps seem not needed.

[JesseCodeBones]

Hmm, what if we'd instead clone the range when calling addDebugLocation, so debugLocations is unique for each function, with each range having its own debugInfoRef? Or perhaps remove the caching entirely, and add debugInfoRef on demand? Haven't looked into this in detail yet, so I might be missing something, it's just that additional maps seem not needed.

yeah, explicit producer and consumer mode is doable, that means functions should manage their range reference and process sequence carefully.
I didnot get the root cause why template function could impact each other, but it is better to increase Robust with datastructure level😁

[JesseCodeBones]

@dcodeIO
we have four options for fixing this issue

• copy Range to each function
  • disadvantage: previously Range follows the AST, this solution will break this principle, and copy action will generate more CPU cost
• create new data structure for debug line info {fileIndex, line, column, expressionPtr, functionExpr}
  • disadvantage: then the debug info will miss the range information, maybe not convenient for developer in future
• change the Range data structure from expressionRef to Map<string, ExpressionRef>
  • disadvantage: new map will bring more memory cost (I think it is OK)
• check why the template function will impact each other and fix it
  • disadvantage: developer need to control the consumer and producer flow carefully, the issue still may happen in future and very hard to debug the root cause

Above is the solutions I can imagine, can you help to figure out which direction we should follow and if you have comments for the direction, please let me know.

[dcodeIO]

Perhaps, would a change like this achieve the desired result?

   /** Adds the debug location of the specified expression at the specified range to the source map. */
   addDebugLocation(expr: ExpressionRef, range: Range): void {
     let targetFunction = this.currentFlow.targetFunction;
     let source = range.source;
     if (source.debugInfoIndex < 0) source.debugInfoIndex = this.module.addDebugInfoFile(source.normalizedPath);
-    range.debugInfoRef = expr;
-    targetFunction.debugLocations.push(range);
+    targetFunction.debugLocations.set(expr, range);
   }

So there is no longer a debugInfoRef on ranges (can be removed), but instead a per-targetFunction mapping of ranges to expressions (here changing debugLocations from an Array<Range> to a Map<ExpressionRef,Range>)?

[Changqing-JING]

If debugLocations is changed to Map, the debugInfoRef of Range can also be remove.

[JesseCodeBones]

@dcodeIO
yeap, this could be a better solution, can you review the change again? Thank you