Computing hover expression has encountered a problem

[jonahgraham]

Describe the bug

Received a pop-up error when hovering mouse over some code while debugging:

To Reproduce Steps to reproduce the behavior:

1. Create a project
2. Make the contents of the source file:

struct s {
    int f;
};

#define macro(a) a

int main(void) {
    struct s x;
    macro(x.f); // hover over the f here
    return 0;
}

3. Add a macro for x=z:

4. Build and start a debug session
5. Hover over the f in the code as indicated in the code comment (don't hover over the f in the comment)

[jonahgraham]

The defined macro (-Dx=z) is required because the error relates to having no image location which has a limited number of use cases, a key one being when the macro comes from the command line as, in this case, the x macro does not exist in any file.

[jonahgraham]

Stack trace of the issue from the error log:

!ENTRY org.eclipse.core.jobs 4 2 2023-01-18 20:07:37.058
!MESSAGE An internal error occurred during: "Computing hover expression".
!STACK 0
java.lang.NullPointerException: Cannot invoke "org.eclipse.cdt.core.dom.ast.IASTImageLocation.getNodeOffset()" because "ownerImageLoc" is null
    at org.eclipse.cdt.debug.ui.editors.AbstractDebugTextHover$1.computeMacroArgumentExtent(AbstractDebugTextHover.java:317)
    at org.eclipse.cdt.debug.ui.editors.AbstractDebugTextHover$1.runOnAST(AbstractDebugTextHover.java:265)
    at org.eclipse.cdt.ui.text.SharedASTJob$1.runOnAST(SharedASTJob.java:74)
    at org.eclipse.cdt.internal.core.model.ASTCache.runOnAST(ASTCache.java:231)
    at org.eclipse.cdt.internal.ui.editor.ASTProvider.runOnAST(ASTProvider.java:338)
    at org.eclipse.cdt.ui.text.SharedASTJob.run(SharedASTJob.java:71)
    at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

[jonahgraham]

252 fixes the NPE, but it won't be able to do the black magic that the code can do when the full location information is available.

https://github.com/eclipse-cdt/cdt/blob/7bcef26f6558aedb6751446f57294505ca8db1ce/debug/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/ui/editors/AbstractDebugTextHover.java#L301

For example, if you use the example from the OP, but leave out the macro definition of x=z, then the hover of f shows this:

However we aren't successfully identifying an expression to pass to GDB when there is no location, so hovering over OP case will show nothing.

Fixing this would be a much bigger change, and #252 does move us forward.

[ddscharfe]

252 fixes the NPE, but it won't be able to do the black magic that the code can do when the full location information is available.

How about some more black magic then ;)

To org.eclipse.cdt.debug.ui.editors.AbstractDebugTextHover::computeMacroArgumentExtent():344

if (ownerImageLoc != null) {
    final int nameOffset = ownerImageLoc.getNodeOffset();
    // offset should be inside macro expansion
    if (nameOffset < startOffset && nameOffset > macroOffset) {
        startOffset = nameOffset;
    }
} 

you could add

} else if (name.getNodeLocations().length == 1
        && name.getNodeLocations()[0] instanceof IASTMacroExpansionLocation expansionLocation) {
    // Node is completely generated within a macro expansion
    IASTPreprocessorMacroExpansion expansion = expansionLocation.getExpansion();
    IASTName[] nestedMacroReferences = expansion.getNestedMacroReferences();

    if (nestedMacroReferences.length == 1) {
        IASTImageLocation imageLocation = nestedMacroReferences[0].getImageLocation();

        if (imageLocation != null) {
            final int nameOffset = imageLocation.getNodeOffset();
            // offset should be inside macro expansion
            if (nameOffset < startOffset && nameOffset > macroOffset) {
                startOffset = nameOffset;
            }
        }
    }
}

I'm not sure about the nestedMacroReferences.length == 1 part though, there are probably cases which this logic won't cover.

[jonahgraham]

Thanks @ddscharfe for the improvement. I added a commit 0b4d3a11868d85f3be4f0a4561b1b406cab281b0 (with you as the author) to the PR. The change does work in the case of the OP. Only change I made was adding a few null checks for API calls that aren't documented as returning non-null.