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

[Petri Latvala]

On Mon, Oct 10, 2022 at 01:32:38PM +0200, Zbigniew Kempczyński wrote:
> 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?

I've been giving a glance now and then and in my understanding the
problem is not as widespread as one might think.

data_t variables are often made global.

Pointer variables are not a problem, for the cases where fixtures
write to where they point to.

Pragmatically, variables in igt_main scope work "correctly" in all
cases, which is weird but hey. Problems arose in the past with
variables introduced in subtest_group scope. See runner/runner_tests.c

Do you have examples in mind that might be a concern?

-- 
Petri Latvala