facebookexperimental / object-introspection

Object Introspection (OI) enables on-demand, hierarchical profiling of objects in arbitrary C/C++ programs with no recompilation.
Apache License 2.0
164 stars 14 forks source link

Handle function drgn type when enumerating types #422

Open tyroguru opened 11 months ago

tyroguru commented 11 months ago

Summary

Handle function types when enumerating types.

Test plan

A test needs adding.

codecov-commenter commented 11 months ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (9e2b48d) 68.70% compared to head (86e97e7) 68.68%. Report is 1 commits behind head on main.

Files Patch % Lines
oi/type_graph/DrgnParser.cpp 0.00% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #422 +/- ## ========================================== - Coverage 68.70% 68.68% -0.03% ========================================== Files 119 119 Lines 11642 11647 +5 Branches 1920 1920 ========================================== + Hits 7999 8000 +1 - Misses 2670 2674 +4 Partials 973 973 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JakeHillion commented 10 months ago

I'm wondering if this change makes total sense - we're taking in a function type which is explicitly unsized, and stating that it's a sized type StubbedPointer. I wonder if it would make more sense to immediately return an Incomplete from this switch case, along with a depth decrement. That would work if the goal is to prevent hitting the catch statement. I could be missing something though!

I wrote this bit of code which I think neatens up the depth decrement, if we do decrement in this specific switch case it would make sense to use something like this: https://github.com/JakeHillion/object-introspection/blob/6be7e7708bfd69c75faa46d42df81a79734eab46/oi/type_graph/ClangTypeParser.cpp#L45-L55

ajor commented 10 months ago

Finding a test case for this proved difficult. Putting this on hold until we hit this issue in production again.