From 64ee6634688d8a49d302b839d6308e2e3a410cbe Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sat, 27 Aug 2022 20:04:07 -0700 Subject: [PATCH] gvpr: fix: wrap members needing cleanup in a struct and outline MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit When seeing this code, the compiler emitted numerous warnings like: gvpr.c: In function ‘gvpr’: gvpr.c:929:17: warning: variable ‘prog’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered] parse_prog *prog = 0; ^~~~ What it is trying to tell us is that this code violates the assumptions of `setjmp`/`longjmp`. Referencing any non-volatile stack-allocated variable after a `longjmp` is undefined behavior.¹ This change extracts all the members that need to be referenced after `longjmp` into a wrapper function. This basically puts a function call boundary in a place such that the code no longer violates assumptions of the C standard. It is a little awkward that we have to do this by hand, as the transformation is mechanical and the compiler technically has enough information to do this for us. But `setjmp`/`longjmp` come from the old world and are low level mechanisms. If the compiler sees a call to them, it assumes you are doing something unusual and want to do it exactly as you wrote. While making this adjustment, some white space is adjusted and some coercion to boolean is changed to explicit comparisons against `NULL`. I do not understand why, but after this change the compiler still believes `nextg` can be clobbered by a `longjmp`. While this is true, `nextg` is not used after `longjmp`-ing back to `gvpr_core`, so I do not know why the compiler thinks it needs to warn us about this. Gitlab: #1801 ¹ This restriction stems from how `setjmp`/`longjmp` work on certain platforms. If a variable is live in a register instead of on the stack, it is not guaranteed to be restored during a `longjmp`. Hence why the value of such variables is undefined after a `longjmp`. --- lib/gvpr/gvpr.c | 195 +++++++++++++++++++++++++++--------------------- 1 file changed, 109 insertions(+), 86 deletions(-) diff --git a/lib/gvpr/gvpr.c b/lib/gvpr/gvpr.c index aee9a484f..8cdec77e8 100644 --- a/lib/gvpr/gvpr.c +++ b/lib/gvpr/gvpr.c @@ -910,8 +910,16 @@ gverrorf (Expr_t *handle, Exdisc_t *discipline, int level, const char *fmt, ...) return 0; } -/* gvpr: - * main loop for gvpr. +/// collective managed state used in \p gvpr_core +typedef struct { + parse_prog *prog; + ingraph_state *ing; + comp_prog *xprog; + Gpr_t *state; + options opts; +} gvpr_state_t; + +/* gvpr_core: * Return 0 on success; non-zero on error. * * FIX/TODO: @@ -921,14 +929,10 @@ gverrorf (Expr_t *handle, Exdisc_t *discipline, int level, const char *fmt, ...) * - do automatic cast for array indices if type is known * - array initialization */ -int gvpr (int argc, char *argv[], gvpropts * uopts) -{ +static int gvpr_core(int argc, char *argv[], gvpropts *uopts, + gvpr_state_t *gs) { Sfdisc_t errdisc; Sfdisc_t outdisc; - parse_prog *prog = 0; - ingraph_state *ing = 0; - comp_prog *xprog = 0; - Gpr_t *state = 0; gpr_info info; int rv = 0; int cleanup, i, incoreGraphs; @@ -941,22 +945,20 @@ int gvpr (int argc, char *argv[], gvpropts * uopts) if (uopts->err) setDisc (sfstderr, &errdisc, uopts->err); } - options opts = scanArgs(argc, argv); - if (opts.state <= 0) { - rv = opts.state; - goto finish; + gs->opts = scanArgs(argc, argv); + if (gs->opts.state <= 0) { + return gs->opts.state; } - if (opts.verbose) + if (gs->opts.verbose) gvstart_timer (); - prog = parseProg(opts.program, opts.useFile); - if (!prog) { - rv = 1; - goto finish; + gs->prog = parseProg(gs->opts.program, gs->opts.useFile); + if (gs->prog == NULL) { + return 1; } - info.outFile = opts.outFile; - info.argc = opts.argc; - info.argv = opts.argv; + info.outFile = gs->opts.outFile; + info.argc = gs->opts.argc; + info.argv = gs->opts.argv; info.errf = gverrorf; if (uopts) info.flags = uopts->flags; @@ -966,20 +968,18 @@ int gvpr (int argc, char *argv[], gvpropts * uopts) info.exitf = 0; else info.exitf = gvexitf; - state = openGPRState(&info); - if (!state) { - rv = 1; - goto finish; + gs->state = openGPRState(&info); + if (gs->state == NULL) { + return 1; } if (uopts->bindings) - addBindings (state, uopts->bindings); - xprog = compileProg(prog, state, opts.compflags); - if (!xprog) { - rv = 1; - goto finish; + addBindings(gs->state, uopts->bindings); + gs->xprog = compileProg(gs->prog, gs->state, gs->opts.compflags); + if (gs->xprog == NULL) { + return 1; } - initGPRState(state, xprog->prog->vm); + initGPRState(gs->state, gs->xprog->prog->vm); if ((uopts->flags & GV_USE_OUTGRAPH)) { uopts->outgraphs = 0; @@ -987,9 +987,9 @@ int gvpr (int argc, char *argv[], gvpropts * uopts) } if (!(uopts->flags & GV_USE_EXIT)) { - state->flags |= GV_USE_JUMP; + gs->state->flags |= GV_USE_JUMP; if ((rv = setjmp (jbuf))) { - goto finish; + return rv; } } @@ -998,99 +998,122 @@ int gvpr (int argc, char *argv[], gvpropts * uopts) else incoreGraphs = 0; - if (opts.verbose) + if (gs->opts.verbose) fprintf(stderr, "Parse/compile/init: %.2f secs.\n", gvelapsed_sec()); /* do begin */ - if (xprog->begin_stmt) - exeval(xprog->prog, xprog->begin_stmt, state); + if (gs->xprog->begin_stmt != NULL) + exeval(gs->xprog->prog, gs->xprog->begin_stmt, gs->state); /* if program is not null */ - if (usesGraph(xprog)) { + if (usesGraph(gs->xprog)) { if (uopts && uopts->ingraphs) - ing = newIngGraphs(0, uopts->ingraphs, &ingDisc); + gs->ing = newIngGraphs(0, uopts->ingraphs, &ingDisc); else - ing = newIng(0, opts.inFiles, &ingDisc); + gs->ing = newIng(0, gs->opts.inFiles, &ingDisc); - if (opts.verbose) gvstart_timer (); - for (state->curgraph = nextGraph(ing); state->curgraph; state->curgraph = nextg) { - if (opts.verbose) fprintf(stderr, "Read graph: %.2f secs.\n", gvelapsed_sec()); - state->infname = fileName(ing); - if (opts.readAhead) - nextg = state->nextgraph = nextGraph(ing); + if (gs->opts.verbose) gvstart_timer (); + for (gs->state->curgraph = nextGraph(gs->ing); gs->state->curgraph; + gs->state->curgraph = nextg) { + if (gs->opts.verbose) fprintf(stderr, "Read graph: %.2f secs.\n", gvelapsed_sec()); + gs->state->infname = fileName(gs->ing); + if (gs->opts.readAhead) + nextg = gs->state->nextgraph = nextGraph(gs->ing); cleanup = 0; - for (i = 0; i < xprog->n_blocks; i++) { - comp_block* bp = xprog->blocks + i; + for (i = 0; i < gs->xprog->n_blocks; i++) { + comp_block* bp = gs->xprog->blocks + i; /* begin graph */ - if (incoreGraphs && (opts.compflags & CLONE)) - state->curgraph = (Agraph_t*)cloneO (0, (Agobj_t*)(state->curgraph)); - state->curobj = (Agobj_t *) state->curgraph; - state->tvroot = 0; + if (incoreGraphs && (gs->opts.compflags & CLONE)) + gs->state->curgraph = (Agraph_t*)cloneO(0, (Agobj_t*)gs->state->curgraph); + gs->state->curobj = (Agobj_t*)gs->state->curgraph; + gs->state->tvroot = 0; if (bp->begg_stmt) - exeval(xprog->prog, bp->begg_stmt, state); + exeval(gs->xprog->prog, bp->begg_stmt, gs->state); /* walk graph */ if (walksGraph(bp)) { - cleanup = traverse(state, xprog->prog, bp, cleanup); + cleanup = traverse(gs->state, gs->xprog->prog, bp, cleanup); } } /* end graph */ - state->curobj = (Agobj_t *) state->curgraph; - if (xprog->endg_stmt) - exeval(xprog->prog, xprog->endg_stmt, state); - if (opts.verbose) fprintf(stderr, "Finish graph: %.2f secs.\n", gvelapsed_sec()); + gs->state->curobj = (Agobj_t*)gs->state->curgraph; + if (gs->xprog->endg_stmt != NULL) + exeval(gs->xprog->prog, gs->xprog->endg_stmt, gs->state); + if (gs->opts.verbose) fprintf(stderr, "Finish graph: %.2f secs.\n", gvelapsed_sec()); /* if $O == $G and $T is empty, delete $T */ - if ((state->outgraph == state->curgraph) && - (state->target) && !agnnodes(state->target)) - agdelete(state->curgraph, state->target); + if (gs->state->outgraph == gs->state->curgraph && + gs->state->target != NULL && !agnnodes(gs->state->target)) + agdelete(gs->state->curgraph, gs->state->target); /* output graph, if necessary * For this, the outgraph must be defined, and either * be non-empty or the -c option was used. */ - if (state->outgraph && (agnnodes(state->outgraph) - || (opts.compflags & SRCOUT))) { + if (gs->state->outgraph != NULL && (agnnodes(gs->state->outgraph) + || (gs->opts.compflags & SRCOUT))) { if (uopts && (uopts->flags & GV_USE_OUTGRAPH)) - addOutputGraph (state, uopts); + addOutputGraph(gs->state, uopts); else - sfioWrite (state->outgraph, opts.outFile, state->dfltIO); + sfioWrite(gs->state->outgraph, gs->opts.outFile, gs->state->dfltIO); } if (!incoreGraphs) - chkClose(state->curgraph); - state->target = 0; - state->outgraph = 0; + chkClose(gs->state->curgraph); + gs->state->target = 0; + gs->state->outgraph = 0; - if (opts.verbose) gvstart_timer (); - if (!opts.readAhead) - nextg = nextGraph(ing); - if (opts.verbose && nextg) fprintf(stderr, "Read graph: %.2f secs.\n", gvelapsed_sec()); + if (gs->opts.verbose) gvstart_timer (); + if (!gs->opts.readAhead) + nextg = nextGraph(gs->ing); + if (gs->opts.verbose && nextg != NULL) { + fprintf(stderr, "Read graph: %.2f secs.\n", gvelapsed_sec()); + } } } /* do end */ - state->curgraph = 0; - state->curobj = 0; - if (xprog->end_stmt) - exeval(xprog->prog, xprog->end_stmt, state); - - finish: - /* free all allocated resources */ - freeParseProg (prog); - freeCompileProg (xprog); - closeGPRState(state); - if (ing) closeIngraph (ing); - freeOpts(opts); + gs->state->curgraph = 0; + gs->state->curobj = 0; + if (gs->xprog->end_stmt != NULL) + exeval(gs->xprog->prog, gs->xprog->end_stmt, gs->state); - if (uopts) { - if (uopts->out) sfdisc (sfstdout, 0); - if (uopts->err) sfdisc (sfstderr, 0); + return 0; +} + +/** main loop for gvpr + * + * \return 0 on success + */ +int gvpr(int argc, char *argv[], gvpropts *uopts) { + gvpr_state_t gvpr_state = {0}; + + // initialize opts to something that makes freeOpts() a no-op if we fail early + gvpr_state.opts.outFile = sfstdout; + + int rv = gvpr_core(argc, argv, uopts, &gvpr_state); + + // free all allocated resources + freeParseProg(gvpr_state.prog); + freeCompileProg(gvpr_state.xprog); + closeGPRState(gvpr_state.state); + if (gvpr_state.ing != NULL) { + closeIngraph(gvpr_state.ing); + } + freeOpts(gvpr_state.opts); + + if (uopts != NULL) { + if (uopts->out != NULL) { + sfdisc(sfstdout, 0); + } + if (uopts->err != NULL) { + sfdisc(sfstderr, 0); } + } - return rv; + return rv; } /** -- 2.40.0