Re: [PATCH 03/15] xen/tools: tracing: several improvements on IRQs tracing

[George Dunlap <george.dunlap@citrix.com>]

On 07/06/17 16:58, Jan Beulich wrote:
>>>> On 07.06.17 at 17:45, <dario.faggioli@citrix.com> wrote:
>> On Wed, 2017-06-07 at 09:05 -0600, Jan Beulich wrote:
>>>>>> On 01.06.17 at 19:33, <dario.faggioli@citrix.com> wrote:
>>>> @@ -884,9 +919,13 @@ void do_IRQ(struct cpu_user_regs *regs)
>>>>              desc->rl_quantum_start = now;
>>>>          }
>>>>  
>>>> -        tsc_in = tb_init_done ? get_cycles() : 0;
>>>> +        if ( unlikely(tb_init_done) )
>>>> +        {
>>>> +            __trace_var(TRC_HW_IRQ_GUEST, 0, sizeof(irq), &irq);
>>>> +            tsc_in = get_cycles();
>>>> +        }
>>>>          __do_IRQ_guest(irq);
>>>> -        TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
>>>> +        trace_irq_handled(irq, get_cycles() - tsc_in);
>>>>          goto out_no_end;
>>>>      }
>>>
>>> Before this change, the get_cycles() invocation was after
>>> the tb_init_done check. Now you move it ahead of it (as
>>> the function arguments are evaluated before executing the
>>> function body) - are you sure all compiler versions will suitably
>>> re-order this?
>>>
>>> Additionally I'm afraid this will trigger compiler warnings on at
>>> least some compiler versions, as tsc_in is now possibly
>>> uninitialized (and there's no clear way to disprove this for the
>>> compiler, again because function arguments are being
>>> evaluated before the function body is reached).
>>>
>> I understand and (now that I see it) agree with your remark on when
>> get_cycles() is called. I'll reorganize things so that the patched
>> behavior matches the existing one.
>>
>> OTOH, I'm not sure I see the "potentially uninitialized" issue for
>> tsc_in, but since I'm reworking the code, I'll keep that in mind too.
> 
> You initialize tsc_in only when tb_init_done is set, but you use
> it without that conditional. And even if you used it under that
> same conditional, older compiler versions aren't able to track
> that fact (same conditional) and raise a warning anyway.

This patch changes thing to initialize tsc_in on declaration (at the top
of the function).

If we want to keep the compiler "uninitialized variable" analysis
around, that would have to go away and we'd want to do something like
was there before.  (I'm ambivalent about it, but I know Jan likes it.)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel