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

Nested named structs names are not detected. #117

Closed
RobertHerreraEECS opened this issue Mar 26, 2023 · 8 comments
Closed

Nested named structs names are not detected. #117

RobertHerreraEECS opened this issue Mar 26, 2023 · 8 comments
Assignees
Labels
bug Confirmed bug

Comments

@RobertHerreraEECS
Copy link

RobertHerreraEECS commented Mar 26, 2023

Currently clang2py (ctypeslib) has some small issues detecting named structs that have been nested in a union.

For my use case the following error can be replicated with the following similar structure:

`struct foo {

union {
	struct {
		u16 a;
		u16 b;
	} STRUCT_PACKED bar;
	struct {
		u16 c;
		u8 d[];
	} STRUCT_PACKED baz;
} u;

}`

The resultant struct that clang2py generates would look something along the lines of the following:

`

  union_foo_0_8_0._pack_ = 1 # source:False
  union_foo_0_8_0._anonymous_ = ('_0', '_1', '_2', '_3', '_4', '_5', '_6', '_7', '_8', '_9', '_10', '_11', '_12', '_13', '_14', '_15',)
 union_foo_0_8_0._fields_ = [
     ('_0', struct_foo_0_8_0_0),
     ('_1', struct_foo_0_8_0_1)]  

`

The generated indicies above are merely an example of real outputted data I'm encoutering but the example does effectively illustrate the issue. In short, I would expect 0_8_0_0 to be bar.

I think either the issue might be that the resultant llvm thats generated by clang is interpreting the struct as an anonymous struct when in fact is a named struct. OR it could be a edge case in the struct decls parsing thats going on under the hood with ctypeslib.

@RobertHerreraEECS
Copy link
Author

FWIW I'm using the following command:

clang2py -v --validate 1 -k mcdefstua --clang-args "-I/dir/includes -std=c11 " target_header.h

@RobertHerreraEECS
Copy link
Author

RobertHerreraEECS commented Mar 26, 2023

Another note, after digging through the logs, the interesting thing is that I can see the actual name of the struct being detected as a field FIELD_DECL, where the expected name in this edited log snippet is bar.

`

2023-03-26 09:38:37,157:DEBUG:clangparser:Unregister foo_0_3
2023-03-26 09:38:37,157:DEBUG:clangparser:register: foo_0_3 
2023-03-26 09:38:37,157:DEBUG:cursorhandler:creating FIELD_DECL for FIELD_DECL/bar
2023-03-26 09:38:37,157:DEBUG:cursorhandler:FIELD_DECL: field offset is 0
2023-03-26 09:38:37,157:DEBUG:cursorhandler:FIELD_DECL: field is 80 bits
2023-03-26 09:38:37,157:DEBUG:cursorhandler:FIELD_DECL: we now look for the declaration name.kind CursorKind.STRUCT_DECL`

@RobertHerreraEECS
Copy link
Author

The following is a simple test showcasing the behavior I'm seeing as well an accompanying c file with the desired use of the struct I would need in my python code.
example.zip

@trolldbois trolldbois added the bug Confirmed bug label Mar 30, 2023
@trolldbois trolldbois self-assigned this Mar 30, 2023
trolldbois added a commit that referenced this issue Mar 30, 2023
@trolldbois
Copy link
Owner

@RobertHerreraEECS can you confirm this is fixed in the main branch ?

@RobertHerreraEECS
Copy link
Author

Thanks for getting this ticket first off!

This seems like a partial fix but definitely much better results. Unfortunately, it seems like the current patch and tests only account for the only 'fields' inheriting the TYPEDECL name and not propagating that into the class namespace as well.

For instance I would expect the following in test case

self.assertNotIn('union_s_0', self.namespace) self.assertIn('union_s_u', self.namespace)

Let me know if this doesn't fit within the scope of functionality ^. If it doesn't, feel free to close the ticket.

Thanks again.

@trolldbois trolldbois reopened this Mar 30, 2023
@trolldbois
Copy link
Owner

@RobertHerreraEECS I'm sorry I do not understand the test case you are proposing.
Could you write python code you see failing / are not happy with, and the C headers ?

@trolldbois
Copy link
Owner

Ah, I think I understand, you mean you would like

  • To not have a type declaration for an anonymous union, that is an un-named field (union_s_0)
  • A type declaration for an anonymous union, that is named field should carry the name of the field (union_s_1 -> union_s_u)

is that what you were asking ?

@RobertHerreraEECS
Copy link
Author

RobertHerreraEECS commented Mar 30, 2023

Yes exactly! "A type declaration for an anonymous union, that is named field should carry the name of the field (union_s_1 -> union_s_u)".

Let me know what you think. I think its quite portable for most use cases. From a C standpoint if I see a structure that is both named and typedef usually the code base resorts to only using the typdef alias.

So my proposition would be if it the structure was unnamed (cursor.type.get_declaration().spelling == '' ) BUT cursor.spelling != '' (the typedef alias), then just use the typedef name rather using a numeric index.

RobertHerreraEECS pushed a commit to RobertHerreraEECS/ctypeslib that referenced this issue Apr 4, 2023
Modified get_unique_name to also account for a the parent node's
typedef name, if one exists. This change is a continuation to trolldbois#117
where the edge cases arise if more than one level of anonymous
typedef'd structs are embedded inside each other.

- Helper function created to determine if parent typdef name exists.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

2 participants