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

Parser improvement #569

Draft
wants to merge 76 commits into
base: master
Choose a base branch
from
Draft

Parser improvement #569

wants to merge 76 commits into from

Conversation

aehyvari
Copy link
Member

This PR, at its core, suggests the use of a more structured parser for smt-lib2. The parser uses when possible std::unique_ptrs for facilitating memory management. In addition it uses structs that improves readability compared to the uniform ASTNode approach used by the previous parser.

In cases where smt-lib instances typically might use very deeply nested structures, the code resorts to raw pointers to avoid stack overflow on destructors of unique_ptrs. For these cases the code uses custom destructors that delete structures in (reverse?) topological order.

The change affects much of the interpret and option code, and I made relatively light refreshing on these parts as well.

It starts to look like using a reentrant parser with custom name is not
supported by flex/bison combination.  This is the last commit where I
still have an attempt at writing the system like that.
It looks like flex + bison combination does not support at the same time
having a custom name for a parser and a reentrant parser.  This commit
removes the custom name for a parser so that we can use reentrancy.
@aehyvari
Copy link
Member Author

At the moment the cluster doesn't let me log in so I haven't tested the code on real instances unfortunately.

On separate benchmarking the parser code seems to be 20% faster compared to previous parser code, so I'm optimistic that this is an improvement. However, the code shouldn't be merged before someone can test it against master in a larger scale.

@aehyvari
Copy link
Member Author

aehyvari commented Nov 1, 2022

QF_UF

non-incremental-QF_UF-6c8b1701-2022-11-01_vs_ecac43e8-2022-11-01

@aehyvari
Copy link
Member Author

aehyvari commented Nov 2, 2022

There are some cases that I believe to be new stack overflows in Interpret that need further studying. I verified instance QF_LIA/20210219-Dartagnan/ReachSafety-Loops/jm2006_variant-O0.smt2. Other suspicious instances are
QF_LIA/20210219-Dartagnan/ReachSafety-Loops/jm2006.c.i.v+cfa-reducer-O0a and QF_LIA/20210219-Dartagnan/ReachSafety-Loops/benchmark10_conjunctive-O0. I'll have a look when I have more time.

non-incremental-QF_LIA-6c8b1701-2022-11-01_vs_ecac43e8-2022-11-01

Copy link
Member

@blishko blishko left a comment

Choose a reason for hiding this comment

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

First quick look. Looks very interesting! I need more time to look at some of the changes in detail.

@@ -145,6 +142,9 @@ int main( int argc, char * argv[] )
opensmt_error( "SMTLIB 1.2 format is not supported in this version, sorry" );
}
else if ( extension != NULL && strcmp( extension, ".smt2" ) == 0 ) {
std::filebuf fb;
fb.open(filename, std::ios::in);
std::istream is(&fb);
Copy link
Member

Choose a reason for hiding this comment

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

What are these added lines doing?

@@ -0,0 +1,101 @@
//
// Created by Antti Hyvärinen on 26.10.22.
//
Copy link
Member

Choose a reason for hiding this comment

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

Use license header

itp_alg_ps, itp_alg_psw, itp_alg_pss, itp_euf_alg_strong,
itp_euf_alg_weak, itp_euf_alg_random, itp_lra_alg_strong,
itp_lra_alg_weak, itp_lra_alg_factor, itp_lra_alg_decomposing_strong,
itp_lra_alg_decomposing_weak};
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this enum should be split into separate enums for propositional, euf and lra interpolation algorithms?

inline void setProduceModels( ) { insertOption(o_produce_models, new SMTOption(1)); }
inline bool setRandomSeed(int seed) { insertOption(o_random_seed, new SMTOption(seed)); return true; }
inline int getRandomSeed ( ) const { return optionTable.find(o_random_seed) != optionTable.end() ? optionTable.at(o_random_seed).getIntVal(): 91648253; }
inline void setProduceModels( ) { insertOption(o_produce_models, SMTOption(1)); }
Copy link
Member

Choose a reason for hiding this comment

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

Should this option have boolean value?

@blishko
Copy link
Member

blishko commented Nov 2, 2022

I don't see any immediate source of new stack overflows. It will be interesting to find out!

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