[GR-38416] Implement debug location info

[adinn]

Implementation PR for issue #4504

This includes location info for parameters and locals that ar ein registers or on the stack.

It does not yet include info for bindings to constants.

Updates to the reference manual will be provided as a separate PR.

[adinn]

I have now implemented location info for locals/params that are bound to primitive constants or a null oop.
I also fixed an error with compiler generated stack slots where I was using the raw offset instead of rebasing with the frame size.

[olpaw]

Would it make sense to simply add an int index field in NativeImageDebugLocalValueInfo instead of managing those indexes externally?

[adinn]

Maybe. ;-)

The reason for making it external is that this is a DWARF-specific property that is being added as an external 'decoration' to the generic model of the code base presented via the DebugInfoProvider and com.oracle.object.debugentry types. The Windows generator might perhaps need to track placement of records associated with locals but, if it does, I wouldn't be able to say whether it needs to do so using an integer offset.

This is actually a special case of a more general problem. The same concern to keep target object-specific decoration separate from generic model types motivates use of the other two external DWARF index classes, DwarfTypeProperties and DwarfClassProperties which are used to decorate TypeEntry, ClassEntry and MethodEntry classes with a variety of different target object-specific properties. These latter cases are more complex not just because there are multiple properties associated with a model object but also because the indexing mechanism can be hierarchical rather than single

I'd like to find a better way to implement decorations that are directly attached to the model objects rather than via all these separate indexes. However, while it might be expedient to simply add a setter to NativeImageDebugLocalValueInfo I feel that would just be a hack for a specific case that did not really address a more general problem.

[olpaw]

I'm curious, does it really happen that when we have hi == otherLo that value.line() != otherValue.line() is ever true?

[adinn]

Yes, it can happen. The line number here is not for the declaration of the local var but for the infopoint where a location was recorded for the local. So, we can have two successive infopoints with different line numbers that both provide values for the local.

However, your question raises a good point all the same. It is important not to merge Range instances with different lines because the Range objects are used to drive generation of debug line info. However, I am now wondering whether we need to care about the line difference when merging location extents. I don't think the debugger lookup for locations requires that the location ranges conform to the line number ranges. So, I think we can remove this check and rely on the subsequent value comparison to decide whether or not to merge the location extents.

[adinn]

This got folded into the equals method where we do have to test for equality of line numbers.

[olpaw]

Testing with svmbuild/hello.helloshows that setting a breakpoint in static method java.lang.Integer#stringSize does not seem to work. If fact this method is not even found with info functions stringSize. If I set a line breakpoint in it I do see something but it looks buggy:

Thread 1 "hello.hello" hit Breakpoint 1, hello.Hello::noInlineManyArgs(int, int, int, int, int, int, int, int, int, float, float, float, float, float, float, float, float, float) (i0=0, i1=1, i2=2, i3=3, i4=4, i5=5, i6=6, i7=7, i8=8, f0=0, f1=1.125, f2=2.25, f3=3.375, f4=4.5, f5=5.625, f6=6.75, f7=7.875, f8=9) at hello/Hello.java:187
187	        System.out.println("i0 = " + i0);
(gdb) s
java.lang.StringConcatHelper::mix(long, int) (lengthCoder=5, value=0) at java/lang/StringConcatHelper.java:112
112	        return checkOverflow(lengthCoder + Integer.stringSize(value));
(gdb) 
java.lang.Integer::stringSize (x=0) at java/lang/Integer.java:551
551	        int d = 1;
(gdb) info locals
d = <optimized out>
p = <error reading variable p (Unhandled dwarf expression opcode 0xff)>
i = <optimized out>

Here are the remaining results of info locals within stringSize

552	        if (x >= 0) {
d = <error reading variable d (Unhandled dwarf expression opcode 0x1)>
p = <error reading variable p (Unhandled dwarf expression opcode 0xff)>
i = <optimized out>
(gdb) 
553	            d = 0;
d = <optimized out>
p = <error reading variable p (Unhandled dwarf expression opcode 0xff)>
i = <optimized out>
(gdb) 
554	            x = -x;
d = <optimized out>
p = <error reading variable p (Unhandled dwarf expression opcode 0xff)>
i = <optimized out>
(gdb) 
556	        int p = -10;
d = <optimized out>
p = <error reading variable p (Unhandled dwarf expression opcode 0xff)>
i = <optimized out>
(gdb) 
557	        for (int i = 1; i < 10; i++) {
d = <optimized out>
p = <error reading variable p (Asked for position 0 of stack, stack only has 0 elements on it.)>
i = <optimized out>
(gdb) 
558	            if (x > p)
d = <optimized out>
p = <error reading variable p (Asked for position 0 of stack, stack only has 0 elements on it.)>
i = <error reading variable i (Unhandled dwarf expression opcode 0x1)>
(gdb) 
559	                return i + d;
d = <optimized out>
p = <error reading variable p (Unhandled dwarf expression opcode 0xff)>
i = <optimized out>

[olpaw]

But many things are working fine. Like e.g.

Thread 1 "hello.hello" hit Breakpoint 2, java.io.PrintStream::println (this=<optimized out>, x=0x7ffff7c06f08) at java/io/PrintStream.java:1027
1027	        if (getClass() == PrintStream.class) {
(gdb) p x->value->len
$3 = 6
(gdb) set print elements 6
(gdb) p/s x->value->data
$4 = 0x7ffff7c06f00 "i4 = 4"...

or

(gdb) b slowPathNewInstance
Breakpoint 1 at 0x469640: file com/oracle/svm/core/genscavenge/ThreadLocalAllocation.java, line 165.
(gdb) r
Starting program: /home/pwoegere/OLabs/issues/GR-38416/graal/substratevm/svmbuild/hello.hello 
...
Breakpoint 1, com.oracle.svm.core.genscavenge.ThreadLocalAllocation::slowPathNewInstance(org.graalvm.compiler.word.Word *) (objectHeader=0xee05a0) at com/oracle/svm/core/genscavenge/ThreadLocalAllocation.java:165
165	        StackOverflowCheck.singleton().makeYellowZoneAvailable();
(gdb) n
167	            DynamicHub hub = ObjectHeaderImpl.getObjectHeaderImpl().dynamicHubFromObjectHeader(objectHeader);
(gdb) 
169	            Object result = slowPathNewInstanceWithoutAllocating(hub);
(gdb) s
com.oracle.svm.core.genscavenge.ThreadLocalAllocation::slowPathNewInstanceWithoutAllocating(java.lang.Class *) (hub=0xee05a0) at com/oracle/svm/core/genscavenge/ThreadLocalAllocation.java:211
211	        DeoptTester.disableDeoptTesting();
(gdb) p/s hub->name->value->len
$1 = 40
(gdb) set print elements 40
(gdb) p/s hub->name->value->data
$2 = 0xc784d0 "java.util.concurrent.locks.ReentrantLock"...

This is really nice 😎

[olpaw]

See review comments. Please also tidy up the commit history (remember commit message start with uppercase letter) and fold cleanup commits into their predecessor commits where it makes sense.

This was just the first round of reviewing. I will have another look mid next week.

[adinn]

@olpaw

Testing with svmbuild/hello.helloshows that setting a breakpoint in static method java.lang.Integer#stringSize does not seem to work. If fact this method is not even found with info functions stringSize. If I set a line breakpoint in it I do see something but it looks buggy:

That's actually normal behaviour for gdb with inline only methods. You can't see them using 'info func' but you can break them. The same behaviour was present before this fix.

(gdb) info locals
d =
p = <error reading variable p (Unhandled dwarf expression opcode 0xff)>

Sorry, all those 'error reading variable' messages are because I messed up the constant loc expression generation at DwarfLocSectionImpl.java:340. I managed to delete the line that inserts a short total byte count before the byte block that encodes the DWARF location expression. I'll fix that in the rework.

[adinn]

@olpaw I addressed all the issues form your original review. While i was fixing the error I introduced into writing of primitive and null constants I also added the code for writing constant object values (i.e. reference to initial heap objects). So, you can now see things like String constants, albeit as references -- you have to execute (gdb) print stringvar->value to see the actual byte array contents.

I'll squash the commits down and check for proper commit messages next.

[adinn]

@olpaw I just added what I believe is the last remaining missing functionality for the localvars patch i.e. I extended the tests to check bindings for constant values.

n.b. It's not actually visible at source debugging level that the test method inlineReceiveConstants is being passed arguments that are bound to constants. Nor is it visible that the locals it computes and prints are also bound to constants. However, I designed the calling and called routines to ensure that they get bound to constants and then verified by stepping through the code that the appropriate numeric and object (String) values are actually loaded from the initial heap. So, I am confident that this test will catch any breakage if the code that assembles the initial heap changes the way they are presented.

[olpaw]

There is a much simpler way to construct GRAAL_AARCH64_TO_DWARF_REG_MAP

b/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/elf/dwarf/DwarfLocSectionImpl.java
--- a/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/elf/dwarf/DwarfLocSectionImpl.java	(revision Staged)
+++ b/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/elf/dwarf/DwarfLocSectionImpl.java	(date 1651762016801)
@@ -599,82 +601,18 @@
         V30(94),
         V31(95);
 
-        public final int encoding;
+        private final int encoding;
+
+        public int getEncoding() {
+            return encoding;
+        }
 
         DwarfRegEncodingAArch64(int encoding) {
             this.encoding = encoding;
         }
     }
 
-    // map from compiler AArch64 register indices to corresponding dwarf AArch64 register index
-    private static final int[] GRAAL_AARCH64_TO_DWARF_REG_MAP = {
-                    DwarfRegEncodingAArch64.R0.encoding,
-                    DwarfRegEncodingAArch64.R1.encoding,
-                    DwarfRegEncodingAArch64.R2.encoding,
-                    DwarfRegEncodingAArch64.R3.encoding,
-                    DwarfRegEncodingAArch64.R4.encoding,
-                    DwarfRegEncodingAArch64.R5.encoding,
-                    DwarfRegEncodingAArch64.R6.encoding,
-                    DwarfRegEncodingAArch64.R7.encoding,
-                    DwarfRegEncodingAArch64.R8.encoding,
-                    DwarfRegEncodingAArch64.R9.encoding,
-                    DwarfRegEncodingAArch64.R10.encoding,
-                    DwarfRegEncodingAArch64.R11.encoding,
-                    DwarfRegEncodingAArch64.R12.encoding,
-                    DwarfRegEncodingAArch64.R13.encoding,
-                    DwarfRegEncodingAArch64.R14.encoding,
-                    DwarfRegEncodingAArch64.R15.encoding,
-                    DwarfRegEncodingAArch64.R16.encoding,
-                    DwarfRegEncodingAArch64.R17.encoding,
-                    DwarfRegEncodingAArch64.R18.encoding,
-                    DwarfRegEncodingAArch64.R19.encoding,
-                    DwarfRegEncodingAArch64.R20.encoding,
-                    DwarfRegEncodingAArch64.R21.encoding,
-                    DwarfRegEncodingAArch64.R22.encoding,
-                    DwarfRegEncodingAArch64.R23.encoding,
-                    DwarfRegEncodingAArch64.R24.encoding,
-                    DwarfRegEncodingAArch64.R25.encoding,
-                    DwarfRegEncodingAArch64.R26.encoding,
-                    DwarfRegEncodingAArch64.R27.encoding,
-                    DwarfRegEncodingAArch64.R28.encoding,
-                    DwarfRegEncodingAArch64.R29.encoding,
-                    DwarfRegEncodingAArch64.R30.encoding,
-                    DwarfRegEncodingAArch64.R31.encoding,
-                    DwarfRegEncodingAArch64.ZR.encoding,
-                    DwarfRegEncodingAArch64.SP.encoding,
-                    DwarfRegEncodingAArch64.V0.encoding,
-                    DwarfRegEncodingAArch64.V1.encoding,
-                    DwarfRegEncodingAArch64.V2.encoding,
-                    DwarfRegEncodingAArch64.V3.encoding,
-                    DwarfRegEncodingAArch64.V4.encoding,
-                    DwarfRegEncodingAArch64.V5.encoding,
-                    DwarfRegEncodingAArch64.V6.encoding,
-                    DwarfRegEncodingAArch64.V7.encoding,
-                    DwarfRegEncodingAArch64.V8.encoding,
-                    DwarfRegEncodingAArch64.V9.encoding,
-                    DwarfRegEncodingAArch64.V10.encoding,
-                    DwarfRegEncodingAArch64.V11.encoding,
-                    DwarfRegEncodingAArch64.V12.encoding,
-                    DwarfRegEncodingAArch64.V13.encoding,
-                    DwarfRegEncodingAArch64.V14.encoding,
-                    DwarfRegEncodingAArch64.V15.encoding,
-                    DwarfRegEncodingAArch64.V16.encoding,
-                    DwarfRegEncodingAArch64.V17.encoding,
-                    DwarfRegEncodingAArch64.V18.encoding,
-                    DwarfRegEncodingAArch64.V19.encoding,
-                    DwarfRegEncodingAArch64.V20.encoding,
-                    DwarfRegEncodingAArch64.V21.encoding,
-                    DwarfRegEncodingAArch64.V22.encoding,
-                    DwarfRegEncodingAArch64.V23.encoding,
-                    DwarfRegEncodingAArch64.V24.encoding,
-                    DwarfRegEncodingAArch64.V25.encoding,
-                    DwarfRegEncodingAArch64.V26.encoding,
-                    DwarfRegEncodingAArch64.V27.encoding,
-                    DwarfRegEncodingAArch64.V28.encoding,
-                    DwarfRegEncodingAArch64.V29.encoding,
-                    DwarfRegEncodingAArch64.V30.encoding,
-                    DwarfRegEncodingAArch64.V31.encoding,
-    };
+    private static final int[] GRAAL_AARCH64_TO_DWARF_REG_MAP = Arrays.stream(DwarfRegEncodingAArch64.values()).mapToInt(DwarfRegEncodingAArch64::getEncoding).toArray();
 
     // register numbers used by DWARF for AMD64 registers
     public enum DwarfRegEncodingAMD64 {

[adinn]

I'm not sure this is the right thing to do. See below for details.

[adinn]

Fixed by documenting the current code as per below.

[olpaw]

Use the same pattern here as shown in https://github.com/oracle/graal/pull/4505/files#r865995904

[adinn]

This doesn't actually work -- it does for AArch64 but only because that architecture has a more sensible naming scheme.

The AArch64 and X86_64 enum tags are sorted by the DWARF encoding. So, for x86_64 the register sequence for DWARF 0...7 is RAX(0), RDX(1), RCX(2), RBX(3), RSI(4), RDI(5), RBP(6), RSP(7).

The Graal register numbers do not follow the same order for their register numbering. For numberings 0...7 the corresponding registers are RAX(0), RCX(1), RDX(2), RBX(3), RSP(4), RBP(5), RSI(6), RDI(7).

That different order is taken account of in the sequence in which the DwarfRegEncoding tags appear in the array initialization expression (notice how the first 8 entries in the following table are permuted). What it boils down to is that there are actually two translation steps at play in this iniitalization. The Graal register number n (range 0-31) indexes an array slot populated using a field expression that accesses the corresponding DwarfRegEncoding enum tag. The access to the encoding field retrieves the DWARF encoding for that DwarfRegEncoding.

This problem could be finessed by re-ordering the enum tags so that they are declared out of DWARF encoding order. If that were done then the iterative enumeration you are suggesting could be made to work. However, I think that would make this already fairly obscure translation process more difficult to follow.

I'd prefer to keep the current layout and add a comment to explain the rationale for this two specific step translation. As it stands, the code explicitly and independently displays each of the two steps in the translation via i) the argument passed to the enum tag and ii) the order in which entries appear in the initializer. Relying on the tag ordering to determine the permutation makes this step implicit and my view is that it would make the details of the mapping harder to understand and to document.

[adinn]

I have pushed a patch that retains the current array initilaizer and documents the steps in the translation. I hope that is ok.

[olpaw]

Relying on the tag ordering to determine the permutation makes this step implicit and my view is that it would make the details of the mapping harder to understand and to document.

Alternatively you could just add another int field to the enum that holds the corresponding Graal register number explicitly for a given enum value (e.g. regNumGraal). Then again turning this into GRAAL_X86_64_TO_DWARF_REG_MAP is just a single line of code (similar to what I suggested before).

[adinn]

Yes, that would be much better. I'll push another update.

[adinn]

Ok, I did as you suggested.

n.b. To make this more bullet-proof I used the named Register instances {r0, ... v31} and {rax ... xmm15} from, respectively, AArch64 and AMD64 to specify the register numbers passed to the corresponding DwarfRegister enum tag. That required adding a dependency to the python suites exposing package jdk.interal.vm.ci/jdk.vm.ci.code to module com.oracle.objectfile . I hope that is ok.

[adinn]

Repushed with format fixes.

[olpaw]

Will this work across different OSes? If we have OSX or Windows instead of Linux?

[adinn]

Doh! No, of course, this needs to employ an OS- and CPU-specific call protocol.

I actually wanted class ParamLocationProducer to employ the calling convention configuration data that drives the GraalVM compiler but I had no idea how to access it. So, instead I simply replicated the relevant info per CPU. Is there some simple way to obtain the compiler config as use it as an input to ParamLocationProducer?

For now I will fix this by multiplying cases in the implementation of ParamLocationProducer to respect both CPU and OS. However, it would be clearly be more 'future-proof' if the data that drives ParamLocationProducer was the same as that used by the compiler.

[adinn]

Class ParamLocationProducer now caters for Linux/AArch64, Linux/X86_64 and WIndows/X86_64. It asserts for other cases because they should never be reached (on OSX the call to installDebugInfo() is fielded by the default implementation on ObjectFile which just returns).