interesting that it was optimized out -- but it does seem to follow the
letter of the standard
references to the volatile variable must access the actual memory and not
be cached
in this case there is no reference because it was optimized out so its easy
is there any standard way to force the increment to happen at any
optimization level?
I think job_unlock() needs 2 more memory barriers
along with volatile unsinged int in_critical or volatile struct jobs job
try this portable code instead:
---
#define job_lock() asoincint(job.in_critical)
#define job_unlock() \
do { \
int _sig; \
if (asodecint(job.in_critical) == 1 && (_sig =
job.savesig)) \
{ \
if (asoincint(job.in_critical) == 0 && !vmbusy()) \
job_reap(_sig); \
asodecint(job.in_critical); \
} \
} while(0)
---
I believe this code with one less aso op is equivalent but I'm not in a
spot to test it right now:
---
#define job_lock() asoincint(job.in_critical)
#define job_unlock() \
do { \
int _sig; \
if (asogetint(job.in_critical) == 1 && (_sig = job.savesig)
&& !vmbusy()) \
job_reap(_sig); \
asodecint(job.in_critical); \
} while(0)
---
On Wed, Jun 25, 2014 at 6:27 AM, Michal Hlavinka <[email protected]>
wrote:
> Hi,
>
> we found a bug caused by compiler optimizations.
>
> The top of stack looks like this:
>
> #0 job_chksave (pid=5066) at jobs.c:1949
> #1 job_reap (sig=17) at obs.c:428
> #2 <signal handler called>
> #3 job_subsave () at sh/jobs.c:1990
> #4 sh_subshell (shp=0x76cba0, t=0x7fd6050c9fe0, flags=4, comsub=3) at
> subshell.c:520
>
> It's based on patched 2012-08-01 so line numbers won't match precisely.
>
> The interesting part is that job_reap was executed during job_subsave
> while job_lock should prevent this:
>
> void *job_subsave(void)
> {
> struct back_save *bp = new_of(struct back_save,0);
> job_lock();
> *bp = bck;
> bp->prev = bck.prev;
> bck.count = 0;
> bck.list = 0; <<---- HERE when signal came
> bck.prev = bp;
> job_unlock();
> return((void*)bp);
> }
>
> when checking the job.in_critical, gdb showed that it is zero
> (gdb) p job.in_critical
> $1 = 0
>
> So the locking was not effective. I've checked the disassembled
> job_subsave and to no surprise:
> │0x429801 <job_subsave+1> mov $0x18,%edi
> │0x429806 <job_subsave+6> callq 0x405de0 <malloc@plt>
> │0x42980b <job_subsave+11> mov 0x343e9e(%rip),%rdx #
> 0x76d6b0 <bck>
> │0x429812 <job_subsave+18> mov %rax,%rbx
> │0x429815 <job_subsave+21> mov 0x343bed(%rip),%eax #
> 0x76d408 <job+40>
> │0x42981b <job_subsave+27> movl $0x0,0x343e8b(%rip) #
> 0x76d6b0 <bck>
> │0x429825 <job_subsave+37> mov %rdx,(%rbx)
> │0x429828 <job_subsave+40> mov 0x343e89(%rip),%rdx #
> 0x76d6b8 <bck+8>
> │0x42982f <job_subsave+47> test %eax,%eax
> │0x429831 <job_subsave+49> movq $0x0,0x343e7c(%rip) #
> 0x76d6b8 <bck+8>
> │0x42983c <job_subsave+60> mov %rdx,0x8(%rbx)
> │0x429840 <job_subsave+64> mov 0x343e79(%rip),%rdx #
> 0x76d6c0 <bck+16>
> │0x429847 <job_subsave+71> mov %rbx,0x343e72(%rip) #
> 0x76d6c0 <bck+16>
> │0x42984e <job_subsave+78> mov %rdx,0x10(%rbx)
> │0x429852 <job_subsave+82> jne 0x429887 <job_subsave+135>
> │0x429854 <job_subsave+84> mov 0x343bb2(%rip),%edi #
> 0x76d40c <job+44>
> │0x42985a <job_subsave+90> test %edi,%edi
> │0x42985c <job_subsave+92> je 0x429887 <job_subsave+135>
> │0x42985e <job_subsave+94> mov 0x341ca3(%rip),%rax #
> 0x76b508 <Vmregion>
> │0x429865 <job_subsave+101> movl $0x1,0x343b99(%rip) #
> 0x76d408 <job+40>
> │0x42986f <job_subsave+111> mov 0x60(%rax),%rdx
> │0x429873 <job_subsave+115> mov $0x1,%eax
> │0x429878 <job_subsave+120> mov (%rdx),%esi
> │0x42987a <job_subsave+122> test %esi,%esi
> │0x42987c <job_subsave+124> je 0x429890 <job_subsave+144>
> │0x42987e <job_subsave+126> sub $0x1,%eax
> │0x429881 <job_subsave+129> mov %eax,0x343b81(%rip) #
> 0x76d408 <job+40>
> │0x429887 <job_subsave+135> mov %rbx,%rax
> │0x42988a <job_subsave+138> pop %rbx
> │0x42988b <job_subsave+139> retq
> │0x42988c <job_subsave+140> nopl 0x0(%rax)
> │0x429890 <job_subsave+144> callq 0x428d60 <job_reap>
> │0x429895 <job_subsave+149> mov 0x343b6d(%rip),%eax #
> 0x76d408 <job+40>
> │0x42989b <job_subsave+155> jmp 0x42987e <job_subsave+126>
>
> there is no job.in_critical AKA <job+40> ++ nor --
>
> Even with volatile attribute, gcc reorders the code so it locks,
> immediately decrements in_critical again as part of unlocking and then do
> the code that's supposed to be lock protected. Adding memory barriers was
> necessary to prevent compiler from reorganizing the code.
>
> See the attached patch.
>
> Michal
>
>
>
> _______________________________________________
> ast-developers mailing list
> [email protected]
> http://lists.research.att.com/mailman/listinfo/ast-developers
>
>
_______________________________________________
ast-developers mailing list
[email protected]
http://lists.research.att.com/mailman/listinfo/ast-developers
