Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debug info for labels to Odin #4385

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

tf2spi
Copy link
Contributor

@tf2spi tf2spi commented Oct 17, 2024

This is a PR that adds debug info for labels in the Odin compiler, so now labels attached to statements in general like if, block, for, etc., have debug info.

package labels
import "core:fmt"
main :: proc() {
        myloop: for i in 0..<5 {
                fmt.println("We have debug labels now!")
        }

        myswitch: switch true {
                case:
                        fmt.println("Debug labels on switches but not in cases")
        }
        myif: if true {
                fmt.println("Debug labels on if statements")
        }
        // etc...
}

This is useful because debuggers can discern the location of code given a function and label and do actions to it like putting breakpoints on it. It's less finnicky than line breakpoints when it comes to source code changes.

# This works on gdb and puts a breakpoint on the 'myif' label in the code above
b labels.main:myif

@tf2spi tf2spi changed the title Add DILabel debug info to Odin Add debug info for labels to Odin Oct 17, 2024
@tf2spi
Copy link
Contributor Author

tf2spi commented Oct 17, 2024

Additional comments:

The way this is done in LLVM is by creating and inserting a DILabel using DIBuilder. There's a bit of a hiccup, though... In C++, this is done via llvm::DIBuilder::createLabel(DIScope, StringRef, DIFile, unsigned, bool) and llvm::DIBuilder::insertLabel(DILabel, DILocation, BasicBlock). However, LLVM-C doesn't wrap and export these functions, so I had to declare these with their mangled names on POSIX and leave these functions unimplemented on Windows.

I've tested the code on Windows, Linux, and Mac OS and it compiles fine. However, I haven't tested all constructs like SOA for loops yet.

);
_ZN4llvm9DIBuilder11insertLabelEPNS_7DILabelEPKNS_10DILocationEPNS_10BasicBlockE(
m->debug_builder,
llvm_label,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: llvm_label could be nullptr here? Although IIRC, LLVM uses new in a bunch of places and doesn't specify std::nothrow, even in the C API, so I think exceptions would kill odin before it returns nullptr anyways.

Comment on lines 1328 to 1331
// TODO(tf2spi): Work with gingerBill to see if we can patch LLVM-C
// to export DILabel functions for the C API on Windows
// or to determine whether it is worthwhile to do this.
// Until then, unimplemented, so just return.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to change the LLVM-C API. It has to be whatever the official one is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, I now realize this is the more wise approach. Better to ask llvm-project to add a new C API that we can eventually use. This ORCv2/LLJIT issue is a fairly recent example of them adding the API fairly quickly when they detect a valid use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My PR adding DebugInfo labels got added recently. Whenever it's convenient and useful to update the LLVM-C DLL for Windows, I can add a case for Windows to use the new functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants