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

Enhance DWARF parser to recognize (and skip) DW_TAG_member entries for static fields #299

Merged
merged 2 commits into from Dec 22, 2016

Conversation

ma-neumann
Copy link
Contributor

Dear dyninst team,

since DWARF3, static fields in C++ classes are represented by DW_TAG_variable entries and (newly added) DW_TAG_member entries. (Happened with GCC 6.2.1 here.) We found this note/issue on it: http://www.dwarfstd.org/ShowIssue.php?issue=161118.1

This seems not to be covered by dyninst yet when parsing structures: they are exposed as fields but with offset -1 in SymtabAPI. If I am not mistaken, fields exposed by SymtabAPI with offset == -1 can then be either constants or static fields.

As these fields are not part of the structures but global variables instead, we added a check to skip these newly added entries during DWARF parsing. What do you think about it?

We stumbled across this when parsing DWARF generated by GCC 6.2.1 from the following (the cyclic dependency was not handled by our code):

class A { ... static A a; ... };

@dyninst-jenkins
Copy link

Can one of the admins verify this patch?

Copy link
Member

@wrwilliams wrwilliams left a comment

Choose a reason for hiding this comment

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

The naming here kind of obfuscates what you're actually testing, and for anything where errors are non-fatal, we prefer return values to outparams (especially when writing predicates like this). So I'd refactor isStaticStructMember a bit.

I think the overall logic we want is more precisely:

  • Do we have a location? Add a field.
  • Is this a named constant? Pass it over to parseConstant so that it will get handled uniformly if/when that gets implemented.
  • Otherwise, skip (with a dwarf_printf indicating that we're doing so). It doesn't matter whether we're skipping this because we expect its location to be defined elsewhere or because we can't do anything with it; fields without offsets are not useful. Never add a field with offset -1.

Thoughts?

…ts (which are fields without location) are forwarded to parseConstant
@ma-neumann
Copy link
Contributor Author

Thanks for the heads up. The logic sounds perfect.

I have removed the new function again and tried to implement it 1:1, please see the second commit.

@wrwilliams
Copy link
Member

ok to test

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.

None yet

3 participants