KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.91k stars 817 forks source link

Clean up line tracking logic for debug info #3496

Closed qingyuanzNV closed 5 months ago

qingyuanzNV commented 5 months ago

This patch cleans up the line tracking logic when debug info is turned on, including:

  1. Add correct file tracking. We used to have sourceFileStringId and currentFileId in the SpvBuilder, but both were always OpString id of the main file. Renamed them to mainFileId and currentFileId and implemented file tracking accordingly.
  2. Make insertion of OpLine/OpDebugLine/OpDebugScope lazy. We used to insert OpLine right after setting the current debug source location, sometimes resulting in multiple cascading OpLines without any meaningful instructions in between. By making the process lazy, an OpLine is only inserted when an instruction is added with a dirty line tracker.
  3. Added per-block line/scope tracker. So whenever an OpLine/OpDebugLine/OpScope is inserted, the state is stored in the Block instead of SpvBuilder. This is how the standard defines the lifetime of such instruction.

Also, some minor changes are:

  1. Skip setting debug source location for linking object at the end of the AST
  2. Rename some functions to better reflect that they are debug info related
  3. No longer generate an OpLine preceding an OpFunction when Non-Semantic Debug is requested. The line information is available via the extension already.

As a result of this patch, we should have non-semantic debug reporting the correct file for variables, functions and .etc. Hopefully, after this patch, we could have more accurate information to fix the file reporting for variables, functions and .etc for non-semantic debug extension. Also, this helps avoid a lot of cascading OpLines for a potential fix for #3403

qingyuanzNV commented 5 months ago

@arcady-lunarg We are trying to catch an internal deadline and put this patch in. Could you help review this PR? Thanks!

arcady-lunarg commented 5 months ago

Yes, I am going to review it today, it also fixes some issues for us. I was at Vulkanised the past few days and didn't get a chance to look at it in enough detail.

On Thu, Feb 8, 2024, 10:04 Qingyuan Zheng @.***> wrote:

@arcady-lunarg https://github.com/arcady-lunarg We are trying to catch an internal deadline and put this patch in. Could you help review this PR? Thanks!

— Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/glslang/pull/3496#issuecomment-1934671448, or unsubscribe https://github.com/notifications/unsubscribe-auth/A5I72BZB5KDWZPUP6Q5OET3YSUHRBAVCNFSM6AAAAABCV4UKA6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZUGY3TCNBUHA . You are receiving this because you were mentioned.Message ID: @.***>

qingyuanzNV commented 5 months ago

Could you help squash and merge with the commit message below (so it's easier for you to review the new changes):

Clean up the debug line info tracking and generation. 
- Correctly populate the field `currentFileId` with the presence of include directive
- Support lazy OpLine/OpDebugLine generation only when a real instruction is added instead of a debug location is set
- Improve the debug location tracking to per-block instead of just per-builder
- A few bug fixes related to debug source info
qingyuanzNV commented 5 months ago

By the way, one important change of this PR is that I added an addInstruction(...) to SpvBuilder to hijack all instructions added to the current build point. You guys might need be alert on this when reviewing new changes to make sure that all new instructions added to the current build point should go through that function instead of directly calling buildPoint.addInstruction(...).