Conversation
| (string option * string * exp) list * | ||
| (* inputs with optional names and constraints *) | ||
| string list * (* register clobbers *) | ||
| string list) option * (* goto locations *) |
There was a problem hiding this comment.
It's not clear from here, why this additional option wrapping is necessary or what the None then means.
michael-schwarz
left a comment
There was a problem hiding this comment.
I think we should also add tests here so we don't accidently break support in the future.
| method! vinst i = | ||
| let handle_inst iosh i = match i with | ||
| | Asm(_,_,slvl,_,_,_) -> List.iter (fun (_,s,lv) -> | ||
| | Asm(_,_,Some(slvl,_,_,_),_) -> List.iter (fun (_,s,lv) -> |
There was a problem hiding this comment.
Is just ignoring the None case the same that we would have had for the dummy statement before?
| @@ -102,7 +102,7 @@ class usedDefsCollectorClass = object(self) | |||
|
|
|||
There was a problem hiding this comment.
Commenting here since I can only comment on the diff: We should check if these zrapp analysis potentially break after this change (quite likely).
Unless we're super sure that they don't break, we should comment that they assume no asm gotos. We mostly have these for historical reasons anyway (came from original CIL), so it's not worth adding proper handling here (as our focus is on Goblint).
Nevertheless, let's add comments that make it clear we have not patched them to support these new constructs.
|
|
||
| asmgoto: | ||
| /* empty */ { [] } | ||
| | COLON asmgotolst { $2 } |
There was a problem hiding this comment.
is just COLON a valid asmgoto?
|
Once we get the remaining open issues addressed, I think it make sense to merge this, even if Goblint will trail behind for a bit, i.e., while the better handling of asm is not yet merged into Goblint. |
I wouldn't rush this so much, but would wait until we have some Goblint PR ready to handle the change to |
|
Closing in favor of PR to be created by students from the Goblint practical which will address this issue in a principled way. |
Implements parsing of
asm gotostatements for #81 and tracking if a asm statement is basic or advanced as required for my thesis.This change requires bottbenj/analyzer@e3af268 in analyzer.
This currently doesn't handle the controlflow resulting from an
asm gotobut should be sufficient for parsing projects that happen to contain such statements.