[igt-dev] [PATCH 01/76] lib: Introduce typed cleanups

[Zbigniew Kempczyński]

On Thu, Oct 06, 2022 at 10:55:48AM +0300, Petri Latvala wrote:
> On Thu, Oct 06, 2022 at 07:28:31AM +0200, Zbigniew Kempczyński wrote:
> > On Wed, Oct 05, 2022 at 10:02:57AM +0200, Mauro Carvalho Chehab wrote:
> > > On Wed, 5 Oct 2022 09:45:32 +0200
> > > Zbigniew Kempczyński <zbigniew.kempczynski at intel.com> wrote:
> > > 
> > > > > +void igt_cleanup_fd(volatile int *fd);
> > > > > +#define igt_fd_t(x__) \
> > > > > +	volatile int x__ cleanup_with(igt_cleanup_fd) = IGT_OUTER_SCOPE_INIT(-1)  
> > > > 
> > > > Why do we need volatile here? Variable after initalization (open) likely won't
> > > > be touched (until exiting its scope) so what's the risk?
> > > 
> > > With current gcc versions, volatile is not needed, but this fails with
> > > clang. The real problem is that C spec doesn't define what happens
> > > with an automatic stack variable after doing a longjmp.
> > 
> > If I'm not wrong longjmp stays in same scope where fd is according to
> > above (GOOD) examples. So I still don't feel what volatile changes in
> > this case apart of directly touching the memory. I would expect that
> > fd will be in same stack frame regardless using volatile keyword. 
> 
> You're assuming optimization keeps it in stack instead of sometimes
> just having it in a register.
> 
> > 
> > +Petri
> > May you elaborate more what's wrong with clang here after removing 
> > volatile?
> 
> In the 'good' example, possibly nothing. But some uses of
> igt_cleanup_fd, especially being able to test it with the bad usage,
> hits the issue.
> 
> There really is no way to get around the issue that assignments to
> stack variables in scopes that you longjmp out of is unspecified in
> the spec. Before this incorrectly becomes a story about gcc vs. clang,
> the weirdness has also been seen with gcc on some particular
> optimization settings.

So we step on thin ice with setjmp/longjmp regardless compiler we're
using. I mean we got no guarantee variable will be properly restored if
compiler will decide so unless volatile enforces re-read of its value
after longjmp. Anyway using volatile for igt_fd_t makes sense but 
what about other variables? To be fully clang ready we should change
all of variables in igt_main scope to be volatile, am I right?

--
Zbigniew

> 
> -- 
> Petri Latvala