Skip to content

Conversation

@ddelnano
Copy link
Member

@ddelnano ddelnano commented Feb 3, 2025

Summary: Deprecate Go dynamic logging

Relevant Issues:

Type of change:

Test Plan:

Changelog Message:

Additional documentation:

…rams (removing golang programs)

Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
@ddelnano ddelnano closed this Feb 3, 2025
ddelnano added a commit that referenced this pull request Feb 11, 2025
)

Summary: Update dynamic logging to work with Go's 1.17+ calling
convention

Despite our earlier messaging that dynamic logging would be deprecated
in #2105, I decided that it was easier to handle the new ABI instead of
deprecating the Go parts of the dynamic tracer. My rationale for this is
that the blocker for achieving feature parity for the new Go ABI,
handling `STRUCT_BLOB` variables, is actually a concern for c/c++
tracepoint programs as well. I felt it was important to keep the dynamic
tracer rather than deprecate it entirely.

As mentioned above, this change does not have full feature parity with
the legacy ABI since `STRUCT_BLOB` variables don't work with register
based calling conventions -- these variables copy a blob of memory with
the assumption that a packed struct exists in a contiguous chunk of
memory. However, this isn't unique to Go binaries as c/c++ applications
can pass certain structs via
[registers](https://github.com/pixie-io/pixie/blob/b0001dab6288508295c54f36c81b3ed80c2677e1/src/stirling/obj_tools/testdata/cc/test_exe.cc#L66-L69)
as well.

This change replaces the previous Go `STRUCT_BLOB` tests with C
equivalents that pass the variable on the stack. When a struct variable
will be passed via a register, dynamic logging will now return an error
and suggest to the user to access struct fields individually. This error
and the test cases that assert the error is returned can be reverted to
their original form once #2106 is complete.

Relevant Issues: #2105

Type of change: /kind cleanup

Test Plan: Existing tests pass and new ones were added where coverage
was needed

Changelog Message: Update Go dynamic logging feature to support Go 1.17+
binaries

---------

Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
ddelnano added a commit to ddelnano/pixie that referenced this pull request Aug 6, 2025
…xie-io#2108)

Summary: Update dynamic logging to work with Go's 1.17+ calling
convention

Despite our earlier messaging that dynamic logging would be deprecated
in pixie-io#2105, I decided that it was easier to handle the new ABI instead of
deprecating the Go parts of the dynamic tracer. My rationale for this is
that the blocker for achieving feature parity for the new Go ABI,
handling `STRUCT_BLOB` variables, is actually a concern for c/c++
tracepoint programs as well. I felt it was important to keep the dynamic
tracer rather than deprecate it entirely.

As mentioned above, this change does not have full feature parity with
the legacy ABI since `STRUCT_BLOB` variables don't work with register
based calling conventions -- these variables copy a blob of memory with
the assumption that a packed struct exists in a contiguous chunk of
memory. However, this isn't unique to Go binaries as c/c++ applications
can pass certain structs via
[registers](https://github.com/pixie-io/pixie/blob/b0001dab6288508295c54f36c81b3ed80c2677e1/src/stirling/obj_tools/testdata/cc/test_exe.cc#L66-L69)
as well.

This change replaces the previous Go `STRUCT_BLOB` tests with C
equivalents that pass the variable on the stack. When a struct variable
will be passed via a register, dynamic logging will now return an error
and suggest to the user to access struct fields individually. This error
and the test cases that assert the error is returned can be reverted to
their original form once pixie-io#2106 is complete.

Relevant Issues: pixie-io#2105

Type of change: /kind cleanup

Test Plan: Existing tests pass and new ones were added where coverage
was needed

Changelog Message: Update Go dynamic logging feature to support Go 1.17+
binaries

---------

Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
GitOrigin-RevId: 8232eee
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.

1 participant