Improve debug context usability

[steve-s]

• document the LeakDetector and hpy_debug fixture
• support activation via env variable HPY_DEBUG_CONTEXT
• support capturing stack traces of handle allocations (for now simple implementation using backtrace)

On high level:

• DebugHandle has new field char *allocation_stacktrace;, filled in when it is created or reused
• debug module has new public functions to disable the stack trace capturing and to set the traces size limit (which implicitly enables the trace capturing)
• by default this is turned off, but __repr__ on DebugHandle tells the user that it can be turned on and how
• when it is turned on, the __repr__ on DebugHandle also includes the stack trace

[steve-s]

@antocuni if you have some spare cycles on your hands, this one is probably best suited for you to review... :-)

[antocuni]

wow, I like this functionality! When it's merged, it's worth a blog post

[antocuni]

maybe this should be put earlier? It looks like HPY_DEBUG_CONTEXT=1 becomes the main and most convenient way to enable the debug mode.

Also, what about calling it HPY_DEBUG?

[steve-s]

Originally I used HPY_DEBUG, but this env variable is also used internally for something else in our setup.py, so I wanted to avoid confusion.

[antocuni]

I think that the user-facing name is more important, so I vote for renaming the variable inside setup.py and use HPY_DEBUG for this

[steve-s]

Makes sense. So the internal one could be HPY_DEBUG_BUILD

[steve-s]

Done, it's HPY_DEBUG and I also added HPY_LOG also described in the docs

[antocuni]

brilliant, I like it!
Maybe for the future, we could add an env variable to enable it?

[antocuni]

you should also show an example of the output, and maybe write a separate section which explains what to expect.
For example, I tried to modify test_stacktrace.c like this:

void bar(void) {
    char *trace;
    create_stacktrace(&trace, 16);
    if (trace != NULL) {
        printf("\n\nTRACE:\n%s\n\n", trace);
        free(trace);
    }
}

void foo(void) {
    bar();
}

void test_create_stacktrace(void)
{
    foo();
}

But by default, it doesn't show the function names:

TRACE:
./test_stacktrace(+0x56a6) [0x563ee1af86a6]
./test_stacktrace(+0x56b6) [0x563ee1af86b6]
./test_stacktrace(+0x3b3c) [0x563ee1af6b3c]
./test_stacktrace(+0x3f43) [0x563ee1af6f43]
./test_stacktrace(+0x52d5) [0x563ee1af82d5]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7ffa2cc0a0b3]
./test_stacktrace(+0x260e) [0x563ee1af560e]

To get the function names, I had to use -rdynamic in the link step of the Makefile, but I had to read to source code and the backtrace manpage to understand why; now I get this:

TRACE:
./test_stacktrace(foo+0xd) [0x560e3e7bc6a6]
./test_stacktrace(test_create_stacktrace+0xd) [0x560e3e7bc6b6]
./test_stacktrace(+0x3b3c) [0x560e3e7bab3c]
./test_stacktrace(+0x3f43) [0x560e3e7baf43]
./test_stacktrace(main+0x231) [0x560e3e7bc2d5]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7f4e595650b3]
./test_stacktrace(_start+0x2e) [0x560e3e7b960e]

Eventually, we should probably switch to libunwind which AFAIK does not require -rdynamic, but for now backtrace is still very valuable!

[steve-s]

Yes, you're right. I am using add2line, which seem to work w/o -rdynamic. Maybe the __repr__ that prints the stack trace could also include a link to the docs where we'd document all those tricks.

[antocuni]

ah, so you get the "raw" stacktrace and convert it manually after it's printed?
This also works, although it would be nice to have it automatically of course.
+1 to add a link to some docs/howto

[antocuni]

I suppose this is fine for now but also probably very slow and memory inefficient because it creates a textual representation of the stack trace for every handle.
I think it's possible to save only the addresses here, and delay the calls to backtrace_symbols and the construction of the textual repr only if/when it's really needed.

[steve-s]

Good point. I wanted to make it generic such that we can easily swap the backend. Thinking about it now, we may also have something like this:

void *stracktrace_create(HPy_ssize_t max_frames_count);
void stacktrace_print(void *stacktrace, char **target, HPy_ssize_t max_frames_count);
void stacktrace_free(void *stacktrace);

but I'll leave that for future work.

[antocuni]

yes, +1 for both your suggestion and for doing it in the future

[steve-s]

@antocuni I believe I addressed the comments and the PR is ready for second round of review

[steve-s]

ping :-)

[steve-s]

@antocuni ping :-)

[antocuni]

I'm fine using the print for now to avoid blocking this PR forever, but eventually we should probably use logging module for that, and HPY_LOG should be used to select the desired log level

[antocuni]

I think this is not needed. HPY_DEBUG was just an internal only thing used by our Makefile, I don't think it's used by anyone else.
And, btw, you should modify the Makefile so that make debug still works :)

[steve-s]

done

[steve-s]

I rebased the the PR because of minor merge conflict. The real changes since the last review are still in the last two commits.

[antocuni]

LGTM, thanks for addressing my comments! Merging it