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

[Arkadiusz Hiler]

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.