[igt-dev] [PATCH i-g-t 2/5] lib/tests/igt_subtest_group: Operate within defined behavior

[Ser, Simon]

On Thu, 2019-05-09 at 14:11 +0300, Arkadiusz Hiler wrote:
> On Thu, May 09, 2019 at 01:31:34PM +0300, Ser, Simon wrote:
> > On Thu, 2019-05-09 at 13:03 +0300, Arkadiusz Hiler wrote:
> > > Quoting lib/igt_core.c:
> > >  * - Code blocks with magic control flow are implemented with setjmp() and
> > >  *   longjmp(). This applies to #igt_fixture and #igt_subtest blocks and all the
> > >  *   three variants to finish test: igt_success(), igt_skip() and igt_fail().
> > >  *   Mostly this is of no concern, except when such a control block changes
> > >  *   stack variables defined in the same function as the control block resides.
> > >  *   Any store/load behaviour after a longjmp() is ill-defined for these
> > >  *   variables. Avoid such code.
> > >  *
> > >  *   Quoting the man page for longjmp():
> > >  *
> > >  *   "The values of automatic variables are unspecified after a call to
> > >  *   longjmp() if they meet all the following criteria:"
> > >  *    - "they are local to the function that made the corresponding setjmp() call;
> > >  *    - "their values are changed between the calls to setjmp() and longjmp(); and
> > >  *    - "they are not declared as volatile."
> > > 
> > > igt_subtest_group test uses two local variables for tracking the state
> > > of execution, making sure that skips are working correctly.
> > > 
> > > We can just make them volatile to be back on the defined grounds.
> > > 
> > > Cc: Simon Ser <simon.ser at intel.com>
> > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > > ---
> > >  lib/tests/igt_subtest_group.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/tests/igt_subtest_group.c b/lib/tests/igt_subtest_group.c
> > > index 7783d021..d8e7d861 100644
> > > --- a/lib/tests/igt_subtest_group.c
> > > +++ b/lib/tests/igt_subtest_group.c
> > > @@ -28,8 +28,8 @@
> > >  
> > >  igt_main
> > >  {
> > > -	bool t1 = false;
> > > -	int t2 = 0;
> > > +	volatile bool t1 = false;
> > > +	volatile int t2 = 0;
> > 
> > Hi,
> > 
> > I think a little comment explaining why these need to be volatile would
> > be a good addition to this patch. Something among the lines of: "Local
> > variables updated in subtests have undefined contents unless marked as
> > volatile".
> 
> Right. I'll add the comment and make Petri read it as the series has his r-b.

Okay. Then with this the patch is:

Reviewed-by: Simon Ser <simon.ser at intel.com>