It not much even about volatile. Yes, volatile can work, but this is beyond
what volatile is meant for.
void foo(void) {
volatile int k;
k = something();
if (setjmp()) {
A:
use(k);
...
}
B:
k = k + 9;
C:
something_which_might_longjmps();
}
Here A and B, for GCC, are two disjoint domains.
At the time A is possibly executing, B cannot possibly have run, so yes,
the use(k) will reload k from stack storage, but nothing tells GCC to flush
k, a local variable, no more accessed after C, to storage.
The original meaning of volatile, meant, always reload from storage.
I am not sure if adding the "always flush to backing storage after modify,
even if no more users follow", is a wise forward looking assumption.
IMO, on top of volatile, it would be wise not to use, within the waserror
section, variables whose values are written after it.
On Fri, Nov 20, 2015 at 9:27 AM, Kevin Klues <[email protected]> wrote:
> On Thu, Nov 19, 2015 at 8:52 PM, 'Davide Libenzi' via Akaros
> <[email protected]> wrote:
> > Didn't the compiler give you a possible uninitialized use (of tsc) at
> line
> > 415 of syscall.c?
>
> I didn't get a warning, though looking at the code, I clearly should have.
>
> > That longjmp based waserror, the more I think, the more it scares me.
> > IMO all the local variables accessed on the longjmp return path, should
> be
> > made volatile.
>
> I've never been 100% comfortable with waserror() myself, but the
> syscall abort mechanism uses error() under the hood, so I have to use
> waserror() here in order to detect an abort and compensate for it. I
> ran what I have by Ron, and he seemed to think it was OK, but it's
> clear there are problems with accessing the computed tsc value, as you
> point out below.
>
> > No matter how hard you tell GCC to clobber registers and memory.
>
> There is no need for a contrived example. If you look at the assembly
> generated for nanosleep itself, it is clearly wrong. Below is the
> code inside the waserror() block. It never even tries to do the
> subtraction of the original tsc (presumably because it decides it's
> value must be 0 at this point, which is a little weird because I never
> initialized it to 0).
>
> ffffffffc204dee4: 0f 31 rdtsc
> ffffffffc204dee6: 48 89 d7 mov %rdx,%rdi
> ffffffffc204dee9: 89 c0 mov %eax,%eax
> ffffffffc204deeb: 48 8d 75 a8 lea -0x58(%rbp),%rsi
> ffffffffc204deef: 48 c1 e7 20 shl $0x20,%rdi
> ffffffffc204def3: 48 09 c7 or %rax,%rdi
> ffffffffc204def6: e8 c5 45 00 00 callq
> ffffffffc20524c0 <tsc2timespec>
>
> Contrast this with when I make it volatile:
>
> ffffffffc204dee4: 0f 31 rdtsc
> ffffffffc204dee6: 48 8b 4d 90 mov -0x70(%rbp),%rcx
> ffffffffc204deea: 48 89 d7 mov %rdx,%rdi
> ffffffffc204deed: 89 c0 mov %eax,%eax
> ffffffffc204deef: 48 c1 e7 20 shl $0x20,%rdi
> ffffffffc204def3: 48 8d 75 a8 lea -0x58(%rbp),%rsi
> ffffffffc204def7: 48 09 c7 or %rax,%rdi
> ffffffffc204defa: 48 29 cf sub %rcx,%rdi
> ffffffffc204defd: e8 de 45 00 00 callq
> ffffffffc20524e0 <tsc2timespec>
>
> It clearly loads the tsc value into %rcx and subtracts it.
>
> > IMO, all the local variable accessed within a waserror() return path, and
> > (re)assigned after the waserror(), must be marked as volatile.
> > Or bad things happen.
>
> Whether it's true in general or not, it's clearly true here. So it's
> good that we went down this path of discussion. However, the better
> answer in this specific case is to move the read_tsc() call above the
> waserror() block, as that provides better timing accuracy anyway.
> When this is done, we get the expected output because the
> 'returns_twice' attribute on the setjmp()in the waserror() macro
> provides the required compiler memory barrier for us.
>
> > One more thing Kevin about the timing tests.
> > These can become jittery with high load, if the test run on a node where
> the
> > test app is not the only thing running.
> > Ask me how I know ... (Jenkins build going red at 0300 in the morning)
> > OTOH some testing needs to be there. What I resorted in the past, was to
> run
> > that test at high priority, do the time();sleep(n);time() sequence 5
> times,
> > and take the lowest of the deltas as sleep time.
> > Not sure we have to care of these cases for Akaros.
>
> By default, all of the utests link in the pthread library, which will
> turn the process into an mcp before the test runs. Since mcps have
> dedicated access to their cores, it's probably not much of an issue on
> Akaros to run at "high priority" compared to Linux. That said, running
> each test 5 times and taking the lowest delta seems reasonable. I've
> updated the tests accordingly.
>
> --
> You received this message because you are subscribed to the Google Groups
> "Akaros" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> To post to this group, send email to [email protected].
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups
"Akaros" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
For more options, visit https://groups.google.com/d/optout.
