Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Segmentation fault error during federated execution caused by multiport #25

Closed
wants to merge 1 commit into from

Conversation

jackyk02
Copy link

This pull request addresses the issue found in the convert_C_port_to_py function. The problem occurs when handling generic_port_instance_struct* cport for multiport instances. Currently, this structure is not properly assigned when dealing with multiport instances, which leads to subsequent issues in the function. On line 433, we see the macro FEDERATED_ASSIGN_FIELDS(((generic_port_capsule_struct*)cap), cport), which relies on cport. In the case of multiport, if cport isn't appropriately assigned, this macro won't function correctly, causing segmentation fault.

Copy link
Contributor

@petervdonovan petervdonovan left a comment

Choose a reason for hiding this comment

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

I left a little question/quibble, but basically I am just happy that it works now. Thank you Jacky! this might have saved me a ton of time.

Comment on lines +415 to +416
generic_port_instance_struct* cport = (generic_port_instance_struct *)port;

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the intended design was to keep this as-is, and to wrap line 433 (FEDERATED_ASSIGN_FIELDS) in if (width == -2). Just guessing since it seems like cport is only being used when the port is not a multiport.

In fact it might have been clearer to move all of the code that touches cport into the branch at line 435-448.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Peter here. The proposed cast doesn't look correct for a multiport, and, although it is probably harmless, it seems misleading.

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

Good diagnosis! I agree with @petervdonovan that there may be a slightly better fix.

Comment on lines +415 to +416
generic_port_instance_struct* cport = (generic_port_instance_struct *)port;

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Peter here. The proposed cast doesn't look correct for a multiport, and, although it is probably harmless, it seems misleading.

@lhstrh
Copy link
Member

lhstrh commented May 21, 2023

@petervdonovan, if you could implement your suggested change and file a PR in reactor-c, that would be great!

@petervdonovan
Copy link
Contributor

Closing as replaced by lf-lang/reactor-c#218

@petervdonovan petervdonovan deleted the multiport branch May 22, 2023 06:55
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants