Repository: compiler-explorer/compiler-explorer (ts)

Issue:
[BUG]: Misplaced label links on demangled inline class members under Clang
### Describe the bug

oddly specific, yes

### Steps to reproduce

- Switch to Clang
- Turn off 'Filter Directives'
- Create a class, a function declared inside the class declaration (for example a constructor), and something that calls that function (for example a global variable of that type)
- Scroll down to that function's .section directive in the output (in the repro link, it's line 27)

### Expected behavior

The 'follow label' link is placed on the demangled symbol

### Observed output

The link is (demangled symbol name length, minus mangled symbol name length) characters too far left

### Reproduction link

https://godbolt.org/z/fz31fs498

### Screenshots

![image](https://github.com/compiler-explorer/compiler-explorer/assets/5085186/ad39fd8c-2ab4-4c7c-be88-a31536aadff5)


### Operating System

_No response_

### Browser version

_No response_

Pull request description:
Fix #6083: rewrite demangleLabels
This code had some problems:
https://github.com/compiler-explorer/compiler-explorer/blob/07d5c3312fce98d2742fa7ed738d4084f3a6975f/lib/demangler/base.ts#L200-L203
There

`PrefixTree.replaceAll` replaces mangled to demangled names 'blindly', with no notion of labels. Later `demangleLabels` runs separate logic to update the column-range of each label in hope of mimicking the `replaceAll` action.  This fails in some cases: 
(1) the `demangleLables` code handles only a single occurrence of each label in a line, 
(2) It also diverges from `replaceAll` in case that a string which is a label-name appears in the line *not* as part of a label (this does happen!).

This fix makes `replaceAll` return detailed account of its actions, and then use these to properly adjust the label ranges.

This fix might not address all the root issues, but it does make some `label` decisions consistent where they weren't.


Patch:
diff --git a/lib/demangler/base.ts b/lib/demangler/base.ts
index b0d8b3435..3fee52651 100644
--- a/lib/demangler/base.ts
+++ b/lib/demangler/base.ts
@@ -84,26 +84,6 @@ export class BaseDemangler extends AsmRegex {
         return !!this.demanglerExe;
     }
 
-    // Iterates over the labels, demangle the label names and updates the start and
-    // end position of the label.
-    protected demangleLabels(labels, tree: PrefixTree) {
-        if (!Array.isArray(labels) || labels.length === 0) return;
-
-        for (const [index, label] of labels.entries()) {
-            const value = label.name;
-            const newValue = tree.findExact(value);
-            if (newValue) {
-                label.name = newValue;
-                label.range.endCol = label.range.startCol + newValue.length;
-
-                // Update the startCol value for each further labels.
-                for (let j = index + 1; j < labels.length; j++) {
-                    labels[j].range.startCol += newValue.length - value.length;
-                }
-            }
-        }
-    }
-
     protected demangleLabelDefinitions(labelDefinitions, translations: [string, string][]) {
         if (!labelDefinitions) return;
 
@@ -196,10 +176,18 @@ export class BaseDemangler extends AsmRegex {
         ].filter(elem => elem[0] !== elem[1]);
         if (translations.length > 0) {
             const tree = new PrefixTree(translations);
-
             for (const asm of this.result.asm) {
-                asm.text = tree.replaceAll(asm.text);
-                this.demangleLabels(asm.labels, tree);
+                const {newText, mapRanges, mapNames} = tree.replaceAll(asm.text);
+                asm.text = newText;
+                // We try to represent in labels.ranges the exact modifications done by replaceAll,
+                // but replaceAll sometimes modifies substrings not recognized as labels :(
+                if (asm.labels) {
+                    for (const label of asm.labels) {
+                        if (mapRanges[label.range.startCol])
+                            label.range = mapRanges[label.range.startCol][label.range.endCol] || label.range;
+                        label.name = mapNames[label.name] || label.name;
+                    }
+                }
             }
 
             this.demangleLabelDefinitions(this.result.labelDefinitions, translations);
diff --git a/lib/demangler/llvm.ts b/lib/demangler/llvm.ts
index c5f7915b3..d55446184 100644
--- a/lib/demangler/llvm.ts
+++ b/lib/demangler/llvm.ts
@@ -81,12 +81,12 @@ export class LLVMIRDemangler extends BaseDemangler {
         if (translations.length > 0) {
             const tree = new PrefixTree(translations);
             for (const [functionName, passes] of Object.entries(passOutput)) {
-                const demangledFunctionName = tree.replaceAll(functionName);
+                const demangledFunctionName = tree.replaceAll(functionName).newText;
                 for (const pass of passes) {
-                    pass.name = tree.replaceAll(pass.name); // needed at least for full module mode
+                    pass.name = tree.replaceAll(pass.name).newText; // needed at least for full module mode
                     for (const dump of [pass.before, pass.after]) {
                         for (const line of dump) {
-                            line.text = tree.replaceAll(line.text);
+                            line.text = tree.replaceAll(line.text).newText;
                         }
                     }
                 }
diff --git a/lib/demangler/prefix-tree.ts b/lib/demangler/prefix-tree.ts
index 6e1ad28d7..9cd414cd4 100644
--- a/lib/demangler/prefix-tree.ts
+++ b/lib/demangler/prefix-tree.ts
@@ -37,6 +37,17 @@ import {assert} from '../assert.js';
 
 type Node = Node[] & {result?: string};
 
+type charRange = {
+    startCol: number;
+    endCol: number;
+};
+
+export type replaceAction = {
+    newText: string;
+    mapRanges: Record<number, Record<number, charRange>>;
+    mapNames: Record<string, string>;
+};
+
 export class PrefixTree {
     root: Node = [];
 
@@ -86,25 +97,47 @@ export class PrefixTree {
     }
 
     // Replace all matches (longest match first) in a line.
-    replaceAll(line: string) {
-        let result = '';
-        let index = 0;
+    replaceAll(line: string): replaceAction {
+        let newText = '';
+        let idxInOld = 0;
+        let idxInNew = 0;
+        const mapRanges = {};
+        const mapNames = {};
         // Loop over each possible replacement point in the line.
         // Use a binary search to find the replacements (allowing a prefix match). If we couldn't find a match, skip
         // on, else use the replacement, and skip by that amount.
-        while (index < line.length) {
-            const lineBit = line.substring(index);
+        while (idxInOld < line.length) {
+            const lineBit = line.substring(idxInOld);
             const [oldValue, newValue] = this.findLongestMatch(lineBit);
             if (oldValue) {
                 // We found a replacement.
-                result += newValue;
-                index += oldValue.length;
+                newText += newValue;
+                mapNames[oldValue] = newValue;
+
+                idxInOld += oldValue.length;
+                idxInNew += newValue.length;
+
+                // The annoying +1 ultimately comes from monaco.Position being 1-based.
+                const oldStart = idxInOld - oldValue.length + 1;
+                const oldEnd = idxInOld + 1;
+                const newStart = idxInNew - newValue.length + 1;
+                const newEnd = idxInNew + 1;
+
+                // JS/TS don't allow using an object such as {oldStart, oldEnd} as a key in a dictionary/Map,
+                // we use a nested dictionary instead.
+                if (!mapRanges[oldStart]) mapRanges[oldStart] = {};
+                mapRanges[oldStart][oldEnd] = {startCol: newStart, endCol: newEnd};
             } else {
                 // No match; output the unmatched character, and keep looking.
-                result += line[index];
-                index++;
+                newText += line[idxInOld];
+                idxInOld++;
+                idxInNew++;
             }
         }
-        return result;
+        return {
+            newText: newText,
+            mapRanges: mapRanges,
+            mapNames: mapNames,
+        };
     }
 }


Test patch:
diff --git a/test/demangler-tests.ts b/test/demangler-tests.ts
index 530e71242..25ed40b6c 100644
--- a/test/demangler-tests.ts
+++ b/test/demangler-tests.ts
@@ -354,27 +354,27 @@ describe('Demangler prefix tree', () => {
     replacements.add('aa', 'long_a');
     replacements.add('aa_shouldnotmatch', 'ERROR');
     it('should replace a short match', () => {
-        expect(replacements.replaceAll('a')).toEqual('short_a');
+        expect(replacements.replaceAll('a').newText).toEqual('short_a');
     });
     it('should replace using the longest match', () => {
-        expect(replacements.replaceAll('aa')).toEqual('long_a');
+        expect(replacements.replaceAll('aa').newText).toEqual('long_a');
     });
     it('should replace using both', () => {
-        expect(replacements.replaceAll('aaa')).toEqual('long_ashort_a');
+        expect(replacements.replaceAll('aaa').newText).toEqual('long_ashort_a');
     });
     it('should replace using both', () => {
-        expect(replacements.replaceAll('a aa a aa')).toEqual('short_a long_a short_a long_a');
+        expect(replacements.replaceAll('a aa a aa').newText).toEqual('short_a long_a short_a long_a');
     });
     it('should work with empty replacements', () => {
-        expect(new PrefixTree([]).replaceAll('Testing 123')).toEqual('Testing 123');
+        expect(new PrefixTree([]).replaceAll('Testing 123').newText).toEqual('Testing 123');
     });
     it('should leave unmatching text alone', () => {
-        expect(replacements.replaceAll('Some text with none of the first letter of the ordered letter list')).toEqual(
-            'Some text with none of the first letter of the ordered letter list',
-        );
+        expect(
+            replacements.replaceAll('Some text with none of the first letter of the ordered letter list').newText,
+        ).toEqual('Some text with none of the first letter of the ordered letter list');
     });
     it('should handle a mixture', () => {
-        expect(replacements.replaceAll('Everyone loves an aardvark')).toEqual(
+        expect(replacements.replaceAll('Everyone loves an aardvark').newText).toEqual(
             'Everyone loves short_an long_ardvshort_ark',
         );
     });