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

Add parsing support for 'asm goto' #150

Closed
wants to merge 1 commit into from

Conversation

emiljapelt
Copy link

@emiljapelt emiljapelt commented Sep 14, 2023

I am working on a project which utilizes goblint-cil, however I have notice a few short comings, which this attempts to fix:

Parts of this were merged as #15.

- Add resetErrors to frontc
- Add __builtin_strcat and __builtin_memchr
- Add support for 'asm goto'
@michael-schwarz
Copy link
Member

michael-schwarz commented Sep 15, 2023

Hi there,

Thank you for the PR!
The new GCC builtins, support for asm inline, and the function to reset the error-state are non-controversial, we'd be happy to merge that.

When it comes to asm goto support, you might want to compare with #92 and #81 to see if there are any differences.

I am personally reluctant about merging this in as is, as it would violate CIL's invariants: instrs are not supposed to have any intraprocedural control flow (https://goblint.github.io/cil/api/goblint-cil/GoblintCil/index.html#type-instr), so asm gotos would have to be promoted to the level of stmts, and prepareCFG and computeCFGInfo would need to be adapted to handle this. Our student estimated this would require changes to ~100 locations in CIL. (#81 (comment)).

If you're up to do this, we happily welcome the contribution.
If you only want such programs to parse but do not need the capabilities to actually analyze them, I would need to discuss with the co-maintainers if a solution where support for parsing is added but all the functions that rely on this throw an exception if the list of gotos is not empty is something we'd be up for merging.

@michael-schwarz
Copy link
Member

Also, it would be good to have some regressions tests that cover the new support you added.

@emiljapelt
Copy link
Author

emiljapelt commented Sep 16, 2023

Alright, I will move the non-controversial contributions to a different branch and create a seperate PR for those (#151).

I will discuss the case of asm goto with my advisors/colleagues, but would currently assume that just parsing without analysis capabilites are fine for our purposes.

As for regressions tests, this is not a term that I am familiar with, so if I could be provides some more context/explaination, I would be happy to give it a go.

@michael-schwarz
Copy link
Member

As for regressions tests, this is not a term that I am familiar with, so if I could be provides some more context/explaination, I would be happy to give it a go.

Yes, I guess we use the terminology in a somewhat non-standard way. What I meant is that it would be nice to add some tests to https://github.com/goblint/cil/tree/develop/test/small1 that could not be parsed before, but can be parsed now.

@michael-schwarz michael-schwarz changed the title Add parsing support for 'asm goto' and 'asm inline', a few gcc builtin's and an error-state reset function Add parsing support for 'asm goto' Sep 19, 2023
@sim642 sim642 self-requested a review September 29, 2023 13:18
@michael-schwarz
Copy link
Member

This seems like it is now subsumed by #161 which we aim to merge soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants