grafana / jfr-parser

Java Flight Recorder parser library written in Go.
Apache License 2.0
43 stars 16 forks source link

Read CPool Constant Index as uint64 #38

Closed sivachandran closed 2 months ago

sivachandran commented 2 months ago

As per https://www.morling.dev/blog/jdk-flight-recorder-file-format/ and https://www.morling.dev/images/jfr_file_format.svg the Constant Index is a 64bit var-int. But the implementation uses 32bit var-int parser. The existing implementation fails with the included jfr which was generated from Java 17.

Now the implementation is changed to read the constant index as 64bit var-int

image

korniltsev commented 2 months ago

Thank you

sivachandran commented 2 months ago

@korniltsev Can you create a tag for the latest version?

korniltsev commented 2 months ago

@sivachandran Sorry for asking after merging, but I have a followup question?

What constant exactly had the issue? And how does it work now?

Don't we need to update IDMap implementation and all the XXXRef types and XXXList.IDMap field as they rely on id being uint32.

For example this code truncates id from the new u64 to u32 back?

id := SymbolRef(v64_)
sivachandran commented 2 months ago

I'm getting the below error without the change

panic: jfr parser ParseEvent error: error reading CP: error reading class{name: jdk.types.DeoptimizationReason, id: 154, fields: [{Name:reason Type:212 ConstantPool:false Array:false}]} int overflow @ 439211

goroutine 1 [running]:
main.main()
        /home/sivachandran/Workspace/personal/jfr-parser/cmd/jfrparser/main.go:62 +0x1a5

It is failing to parse jdk.types.DeoptimizationReason from ConstantPool. The type is unknown to the implementation. So it tries to parse using SkipConstantPool struct. While parsing SkipConstantPool it fails parsing the constant index.

Don't we need to update IDMap implementation and all the XXXRef types and XXXList.IDMap field as they rely on id being uint32.

Yes, we should. I failed to realise. I will raise a separate PR for those changes.

sivachandran commented 2 months ago

The test didn't fail as the parsed type is not really used anywhere.

korniltsev commented 2 months ago

Great, thank you so much.