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

[Zbigniew Kempczyński]

On Mon, Oct 10, 2022 at 06:36:29PM +0300, Petri Latvala wrote:
> 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

I lack knowledge about mechanics under the hood (compiler and libc
implementation). I didn't previously look into runner_tests.c code,
now it's clear what is its genesis.

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

Not at the moment but I'm going to play with the setjmp()/longjmp()
to understand possible threats.

--
Zbigniew

> 
> -- 
> Petri Latvala