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

Environments in the C target #1772

Merged
merged 153 commits into from
Jun 17, 2023
Merged

Environments in the C target #1772

merged 153 commits into from
Jun 17, 2023

Conversation

erlingrj
Copy link
Collaborator

@erlingrj erlingrj commented May 23, 2023

This PR is (mostly) about introducing the environment concept in reactor-c. There are also changes related to enclaves (mostly contained in CEnclavedReactorTransformation.java). This PR is mainly about:

  1. Code-generating some functions related to creating and initializing the environments of the program
  2. Move global variables tracking things like, all the timers, all startup reactions, shutdown reactions, etc onto the environment.
  3. Changes in code-generation of _lf_initialize_trigger_objects so that we incoroprate the move from global variables mentioned in (2) to variables on the environment struct.

Companion reactor-c PR: lf-lang/reactor-c#212

@erlingrj erlingrj marked this pull request as draft May 23, 2023 22:26
@lhstrh lhstrh changed the title Draft: Scheduling enclaves in the C target Scheduling enclaves in the C target May 23, 2023
@erlingrj erlingrj marked this pull request as ready for review June 9, 2023 14:50
@lhstrh lhstrh changed the title Add environments to the C-target Environments in the C target Jun 10, 2023
@erlingrj
Copy link
Collaborator Author

@petervdonovan @lhstrh @edwardalee I have now updated the tracing API also to deal with multiple environments. This PR and lf-lang/reactor-c#212 are now ready for review

Copy link
Collaborator

@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.

Spot check looked good to me. Let's get this merged!

Copy link
Collaborator

@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 did not understand everything going on here, but I did not spot any important issues.

test/C/c/sendreceive.c Show resolved Hide resolved
core/src/main/java/org/lflang/generator/c/CUtil.java Outdated Show resolved Hide resolved
@erlingrj
Copy link
Collaborator Author

Thanks for your feedback! I have addressed it now. I think @petervdonovan raises an important point about the pollution of the namespaces. I have an issue on this here: lf-lang/reactor-c#237. I can commit to addressing this issue next. I would like to merge these two big PRs now and rather address the re-architecting of the platform-support in a separate PR.

What do you think?

@petervdonovan
Copy link
Collaborator

What do you think?

I 100% agree. I do not want the namespace problems to hold up this PR and would be willing to help with them later

@erlingrj
Copy link
Collaborator Author

Lets merge this, @lhstrh ?

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

LGTM (there is a comment staged from an earlier look at this, but I don't know what it is, so we'll see as soon as I hit the "approve" button).

.gitignore Outdated Show resolved Hide resolved
@erlingrj
Copy link
Collaborator Author

I dont have permissions to merge the branch or add it to the merge queue. I can only keep updating the branch which triggers a new run of the tests.

@lhstrh lhstrh added this pull request to the merge queue Jun 17, 2023
Merged via the queue into master with commit ea65c7a Jun 17, 2023
@lhstrh lhstrh deleted the enclaves branch June 17, 2023 04:01
@lhstrh lhstrh added the feature New feature label Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants