I'm marking this as a draft because I would like to get feedback before updating downstream packages.

This PR requires the following change in rclcpp, everywhere TRACEPOINT(rclcpp_callback_register, ...) is called:

+ char * symbol = tracetools::get_symbol(callback_);
  TRACEPOINT(
    rclcpp_callback_register,
    static_cast<const void *>(&callback_),
-   tracetools::get_symbol(callback_));
+   symbol);
+ std::free(symbol);

Of course, we can surround this with #ifndef TRACETOOLS_DISABLED/#endif to avoid calling tracetools::get_symbol() and allocating memory if the instrumentation is completely removed. However, if the instrumentation is included, then, even if the tracepoint is disabled (i.e., even if the data isn't actually collected by the tracer), memory will be allocated.

To solve this, we can replace LTTng's tracepoint(...) with:

if (tracepoint_enabled(name)) {
  // Compute arguments here
  auto args = ...;
  // Then trigger tracepoint manually
  do_tracepoint(name, args);
}

See lttng-ust(3). Then we can first check if the tracepoint is enabled before calling tracetools::get_symbol()/allocating, triggering the tracepoint manually, and freeing the memory. However, this would mean having to create the corresponding macros to go alongside the current TRACEPOINT(...) macro, i.e., TRACEPOINT_ENABLED(...) and DO_TRACEPOINT(...). This complicates tracetools a bit, but it allows for more flexibility for things like this.

cc @iluetkeb @mjcarroll

Good catch!

The current approach basically passes on the obligation to free the memory to the caller of get_symbol. This is the most straightforward approach, but I'm wondering whether we can come up with something to avoid it. You have thought about the pre-allocated buffer (but we would also have to free that somehow).

btw, one of the other thoughts I had is whether we could output metadata throughout execution at regular intervals. This would then require us to store the metadata anyhow. I'm not saying we need that, but it's maybe something to consider in conjunction.

The current approach basically passes on the obligation to free the memory to the caller of get_symbol. This is the most straightforward approach, but I'm wondering whether we can come up with something to avoid it. You have thought about the pre-allocated buffer (but we would also have to free that somehow).

demangle_symbol() could have its own buffer (on the stack), and:

1. Call abi::__cxa_demangle, let it allocate
2. Copy the result into its buffer, up to some max length
   • I could come up with a reasonable value; in any case this isn't really critical
3. Free the string returned by abi::__cxa_demangle
4. Return a const char * to the buffer
   • So tracetools::get_symbol() and friends would return a const char *

This wouldn't really work if you called tracetools::get_symbol() more than once before the tracer can read the first string, but we might not need to call it consecutively anyway, and we could document this. This makes it easier for the caller, but this might be too weird of a behaviour. As you said, the simpler approach is to let the caller free the memory.

btw, one of the other thoughts I had is whether we could output metadata throughout execution at regular intervals. This would then require us to store the metadata anyhow. I'm not saying we need that, but it's maybe something to consider in conjunction.

Yeah, we should keep these things separate for now. I opened an issue for this an mentioned that as a potential solution: #44.

However, this would mean having to create the corresponding macros to go alongside the current TRACEPOINT(...) macro, i.e., TRACEPOINT_ENABLED(...) and DO_TRACEPOINT(...). This complicates tracetools a bit, but it allows for more flexibility for things like this.

Note that this improvement in tracetools (and downstream changes in rclcpp) could be made in separate PRs.

Testing --packages-above tracetools test_tracetools rclcpp:

• Linux
• Linux-aarch64
• Windows

Includes:

• This PR
• Fix memory leak in tracetools::get_symbol() rclcpp#2104

@mjcarroll friendly ping

I think this caused a fair number of regressions. This repository's CI shows breakage, and my builds on RHEL 8 are now failing as well.

Stuff like:

libtracetools.so: undefined reference to `dladdr'
collect2: error: ld returned 1 exit status

@Crola1702 FYI

I think this caused a fair number of regressions. This repository's CI shows breakage, and my builds on RHEL 8 are now failing as well.

Stuff like:

libtracetools.so: undefined reference to `dladdr'

collect2: error: ld returned 1 exit status

Are there any non-RHEL failures?

Note that this broke ABI, so some Rpr failures are expected. But not the one you included above; that one does make sense to me, especially given that we didn't test this change on RHEL.

Also, don't mind the local GitHub actions. They're broken.

Looking at CI, it seems to be RHEL-only (at least regarding this PR). #48 fixes that.