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

Boogie Verifier instrumentation for generating AST-to-CFG validation certificates #4

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

ahubanov-eth2
Copy link
Collaborator

@ahubanov-eth2 ahubanov-eth2 commented Aug 29, 2022

ObjectExtensions.cs file in the Core module is redundant, added reference to ProofGenUtil from Core.

@gauravpartha
Copy link
Owner

There seems to be often lots of redundant white space in the generated proofs. For example

grafik

@gauravpartha
Copy link
Owner

It would be good to add comments to the newly added code such that it is easier to understand the code. Moreover, the parts where big blocks are added for the purpose of proof generation should be documented clearly.


namespace Core
{
public static class ObjectExtensions
Copy link
Owner

Choose a reason for hiding this comment

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

This file is duplicated (also in ProofGenUtil). It should be possible to keep this file only once, since there are no dependencies from this file to the Boogie code.

Comment on lines 17 to 24
/*
if (mapFromImplementationToProofGenInfo == null)
{
mapFromImplementationToProofGenInfo = new Dictionary<Implementation, ProofGenInfo>();
}

return mapFromImplementationToProofGenInfo;
*/
Copy link
Owner

Choose a reason for hiding this comment

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

should this be removed?

Comment on lines 566 to 567
impl = new Implementation(x, x.val, typeParams.ConvertAll(tp => new TypeVariable(tp.tok, tp.Name)),
impl = new Implementation(x, x.val, typeParams.ConvertAll(tp => new TypeVariable(tp.tok, tp.Name)),
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like there is no change here. It would be good to keep the old formatting so that no change is displayed.

@@ -581,7 +582,7 @@ public override int ComputeHashCode()
Expect(35);
ProcSignature(false, out x, out typeParams, out ins, out outs, out kv);
ImplBody(out locals, out stmtList);
impl = new Implementation(x, x.val, typeParams, ins, outs, locals, stmtList, kv, this.errors);
impl = new Implementation(x, x.val, typeParams, ins, outs, locals, stmtList, kv, this.errors);
Copy link
Owner

Choose a reason for hiding this comment

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

It seems like there is no change here. It would be good to keep the old formatting so that no change is displayed.

Comment on lines 507 to 521
CoalesceBlocks(program);
//CoalesceBlocks(program);
Copy link
Owner

Choose a reason for hiding this comment

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

Block coalescing should be left switched on.

Copy link
Owner

Choose a reason for hiding this comment

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

What impact does this have on the evaluation?

@@ -484,6 +602,31 @@ public static Term RedCmdList(BoogieContextIsa boogieContext, Term cmdList, Term
finalCFGConfig
});
}

public static Term LoopIH(BoogieContextIsa astBoogieContext, BoogieContextIsa cfgBoogieContext, Term ast, Term bigblock, Term cont, Term cfgBody, Term blockIndex, Term posts)
Copy link
Owner

Choose a reason for hiding this comment

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

A more specific name than LoopIH would be useful.

Comment on lines 153 to 160
public MembershipLemmaManager(
IsaProgramGeneratorConfig config,
IsaProgramRepr isaProgramRepr,
IsaBigBlockInfo isaBigBlockInfo,
Tuple<int, int> GlobalsMaxLocalsMin,
IVariableTranslationFactory factory,
string theoryName
)
Copy link
Owner

Choose a reason for hiding this comment

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

this constructor seems to share a lot of code with the previous one. It would be good to share that code.

Comment on lines 49 to 55
foreach (var id in paramsAndLocalMapping.Keys)
{
if (variable.ToString() == id.ToString())
{
variable = id;
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand the point of this code, it does not seem to have an effect.

Comment on lines 42 to 44
IProgramAccessor programAccessor,
IProgramAccessor beforeCfgProgramAccessor,
CFGRepr cfg)
Copy link
Owner

@gauravpartha gauravpartha Sep 18, 2022

Choose a reason for hiding this comment

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

What was the reason again for requiring two program accessors here? It would be good if you could describe the reasons why this is required.

//TODO: here assuming that we use "'a" for the abstract value type carrier t --> make t a parameter somewhere
new TermWithExplicitType(new TermIdent(typeInterpId), IsaBoogieType.AbstractValueTyFunType(new VarType("a"))),
funDecls,
constantDecls,
globalDecls,
axioms,
procedure));
procedure,
//TODO: define this elsewhere.
Copy link
Owner

Choose a reason for hiding this comment

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

resolve

ahubanov-eth2 and others added 24 commits October 5, 2022 08:07
…anup, still need to run and check all examples
the previous versoin used explicit symbols, which cannot be parsed by Isabelle's command line tool
# Conflicts:
#	Source/ProofGeneration/BoogieIsaInterface/IsaBoogieTerm.cs
(i.e., revert Parser.cs to commit ce40e35, which was the last non-proof-generation commit for Parser.cs from the main Boogie repository)
…variable declarations before and after optimizations

The change also adjusts the selective phase generation. In particular, one does not need to give the AstToCfg program accessor to phases that are not connected to it. This is achieved by making the AstToCfg program accessor a parent of those program accessors right below it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants