Order "begin" probes are run

Order "begin" probes are run

[Mike Mason]

It appears that "begin" probes run in no particular order.  Is that true?  

I have a tapset that uses a "begin" probe to do initialization.  I also have a script with a "begin" probe that depends on the tapset's "begin" probe having already been run.  Unfortunately, the script's "begin" probe runs first and it fails.  

Seems reasonable to expect tapset "begin" probes to always run before a script's "begin" probe. Otherwise we can't rely on "begin" probes for initialization.  What do others think?

Mike

Re: Order "begin" probes are run

[Frank Ch. Eigler]

Mike Mason <mmlnx@us.ibm.com> writes:

> It appears that "begin" probes run in no particular order.  [...]

Right.

> Seems reasonable to expect tapset "begin" probes to always run
> before a script's "begin" probe. [...]

One might also imagine cases where it could work the other way.

We could solve this by parametrizing: adding a sequence parameter to
"probe begin(N)" (and "end(M)"), and sorting them.  Easy to implement.

- FChE

RE: Order "begin" probes are run

[Stone, Joshua I]

On Monday, December 11, 2006 11:38 AM, Frank Ch. Eigler wrote:
> Yes, this is a good point.  In other tools in a similar context, we
> ended up letting numeric operands span from the negative signed limit
> (-2**63+1) to the positive unsigned limit (2**64-1).  Shall we?  For a
> small extra bribe, -2**63 could be fine too.  But let's reject numbers
> outside that range.

Done -- it now allows [-2**63 .. 2**64-1].


Josh

Re: Order "begin" probes are run

[Frank Ch. Eigler]

Hi -

On Mon, Dec 11, 2006 at 11:29:37AM -0800, Stone, Joshua I wrote:
> [...]
> Ok.  I checked in code on Friday to move to C-style initialization, and
> I think my changes made this ordering problem better as well [...]

Thanks.

> This would be fine if our numbers were always signed, but we have a
> bit of schizophrenia in that regard.  [...]

Yes, this is a good point.  In other tools in a similar context, we
ended up letting numeric operands span from the negative signed limit
(-2**63+1) to the positive unsigned limit (2**64-1).  Shall we?  For a
small extra bribe, -2**63 could be fine too.  But let's reject numbers
outside that range. 

- FChE

RE: Order "begin" probes are run

[Stone, Joshua I]

On Monday, December 11, 2006 8:19 AM, Frank Ch. Eigler wrote:
> For some time now, we have had code to set some script globals at
> module-load using module options.  This initialization step should
> probably be defined to take place at priority 0, but uses a different
> mechanism.  Thus, the probe-based and module-parameter-based
> initializations could have ordering problems.

Ok.  I checked in code on Friday to move to C-style initialization, and
I think my changes made this ordering problem better as well, or at
least more predictable.  The module startup now goes something like
this:
  1. Initial load -- globals are initalized.
  2. Module parameters set -- may override global values from #1.
  3. Module initialization -- runs ordered begin probes, which may do
anything.


> I would prefer the parser to accept -2**63+1 ... 2**63-1 for literals
> everywhere.

This would be fine if our numbers were always signed, but we have a bit
of schizophrenia in that regard.  Take kernel.statement for example --
we really want an unsigned literal, as 2**63-1 is too low for kernel
addresses on 64-bit platforms.

We could special case it for hex values: allow the full range of
uint64_t for hex literals, and otherwise limit it as you suggest.  Would
this be ok?  And if so, do we allow '-0x...' too?


Josh

Re: Order "begin" probes are run

[Frank Ch. Eigler]

Hi -

On Fri, Dec 08, 2006 at 12:18:32PM -0800, Stone, Joshua I wrote:
> [...]
> Well, changing it to C-style initialization should be easy enough, and
> seems more intuitive to me.  

OK.

> I'm not sure I understand the ordering conflict you're talking about
> though.

For some time now, we have had code to set some script globals at
module-load using module options.  This initialization step should
probably be defined to take place at priority 0, but uses a different
mechanism.  Thus, the probe-based and module-parameter-based
initializations could have ordering problems.


> Yeah, I know.  It wraps around on the positive side too --
> 9223372036854775808 becomes -9223372036854775808.  [...]

That's a little less bad.

> The same problem exists in other parts of the language as well:
> $ ./stap -e 'probe begin { printf("%d\n", -9223372036854775809) exit()}'
> 9223372036854775807

This is another one of those cases where I could've sworn that there
was a parseko test case.

> [...]  It's not obvious to me that there's a right thing for the
> translator to do in either case.  It's up to the programmer to know
> that there are limits to 64-bit numbers.

I would prefer the parser to accept -2**63+1 ... 2**63-1 for literals
everywhere.  To get -2**63, one would have to use the same construct
that you will find in <limits.h> for foo_MIN values, e.g.:

   #define INT_MIN       (-INT_MAX-1)

This would make that value unsuitable as a strict literal.  This could
in turn come in handy as an encoding of the explicit 

   global var=5

initialization.

- FChE

Re: Order "begin" probes are run

[Li Guanglei]

Stone, Joshua I wrote:

> This is now implemented -- you can give a numeric parameter to begin/end
> probes, and they will execute in increasing order.  The sequence number
> if left out is effectively zero.
> 
> We should adopt a convention for tapset writers to use these fields.  My
> suggestion: when it doesn't matter, just use begin/end without a
> parameter; to run early, use less than -1000; and to run late use
> greater than 1000.  This way script writers still have +/- 1000 to play
> with locally.
> 
> 
> Josh

Sorry for the late response. This is really a useful feature for LKET 
since LKET will perform some initialization work and without this 
feature it's hard to control the running order of begin probes.

- Guanglei

RE: Order "begin" probes are run

[Stone, Joshua I]

On Friday, December 08, 2006 11:54 AM, fche@redhat.com wrote:
> "Stone, Joshua I" <joshua.i.stone@intel.com> writes:
>> [...]  Even changing [global initialization] to begin(-2^63) isn't
>> really correct, as it should happen *before* the beginning.  [...]
> 
> -2^63 is not actually a bad choice, and easily documented.  The more
> interesting ordering conflict is between this and
> module-parameter-based initialization.

Well, changing it to C-style initialization should be easy enough, and
seems more intuitive to me.  I'm not sure I understand the ordering
conflict you're talking about though.
 
> By the way, the new parse_literal variant does not check for numeric
> overflow for negative numbers quite right (running on x86-64):
> 
> % ./stap -p1 -e 'probe
> begin(-9223372036854775808),begin(-9223372036854775809) {}' # parse
> tree dump # file <input>
> probe begin(-9223372036854775808),
> begin(9223372036854775807){
> }

Yeah, I know.  It wraps around on the positive side too --
9223372036854775808 becomes -9223372036854775808.  (Side note - that's
one of the answers to an interesting CS question: what are the *two*
values of 'int64_t x' where x == -x?)

The same problem exists in other parts of the language as well:

$ ./stap -e 'probe begin { printf("%d\n", -9223372036854775809) exit()}'
9223372036854775807

In that case, it's being parsed as a positive that wraps around, and
then operated by a unary minus.  

It's not obvious to me that there's a right thing for the translator to
do in either case.  It's up to the programmer to know that there are
limits to 64-bit numbers.


Josh

Re: Order "begin" probes are run

[Frank Ch. Eigler]

"Stone, Joshua I" <joshua.i.stone@intel.com> writes:

> [...]  Well, I implemented the check for negative in parse_literal,
> which is near the bottom of the parsing stack.  [...]  If you can
> think of a test case where this fails, please let me know. [...]

I couldn't find a problem case with a few minutes' effort, so OK.

> [...]  Even changing [global initialization] to begin(-2^63) isn't
> really correct, as it should happen *before* the beginning.  [...]

-2^63 is not actually a bad choice, and easily documented.  The more
interesting ordering conflict is between this and
module-parameter-based initialization.

By the way, the new parse_literal variant does not check for numeric
overflow for negative numbers quite right (running on x86-64):

% ./stap -p1 -e 'probe begin(-9223372036854775808),begin(-9223372036854775809) {}'
# parse tree dump
# file <input>
probe begin(-9223372036854775808),
begin(9223372036854775807){
}

- FChE

RE: Order "begin" probes are run

[Stone, Joshua I]

On Thursday, December 07, 2006 7:02 PM, Frank Ch. Eigler wrote:
> The change to unary number parsing is probably suspect though, in that
> it introduces parsing errors or ambiguities.  I thought we had a test
> for some instances of this problem.

There's definitely a token abiguity, as already noted in the lexer
comments -- should "1-1" be '1'+'-'+'1' or '1'+'-1'?  That's why I
didn't make the change in the lexer.

> If there is any doubt, I would suggest rolling this back, and let the
> probe point parser accept an explicit negation operator as an optional
> prefix.

Well, I implemented the check for negative in parse_literal, which is
near the bottom of the parsing stack.  There are three contexts where
this is called: global initialization and probe point parameters, where
binary operators aren't allowed; and parse_value, where both binary and
unary operators have already been accounted for.  If you can think of a
test case where this fails, please let me know.

We now get the bonus that global numeric initializers can be negative as
well.

However, I just noticed a bug, or at least surprise behavior, in the way
globals are initialized:

    global a = 42
    probe begin(-1), begin(1) { printf("%d ", ++a)

This will print "1 43 ".  It's not really a problem with the sequencing,
but it's because "global a=42" gets synthesized to "global a  probe
begin{a=42}" -- so the order that the initialization happens is
generally undefined.  Even changing it to begin(-2^63) isn't really
correct, as it should happen *before* the beginning.  I will file a bug
and investigate this.  There's a comment indicating that a C-initializer
might be the way to go, so I'll probably go that route.


Josh

Re: Order "begin" probes are run

[Mike Mason]

Stone, Joshua I wrote:
> On Wednesday, November 29, 2006 1:57 PM, Stone, Joshua I wrote:
>> On Wednesday, November 29, 2006 1:23 PM, Frank Ch. Eigler wrote:
>>> Mike Mason <mmlnx@us.ibm.com> writes:
>>>> Seems reasonable to expect tapset "begin" probes to always run
>>>> before a script's "begin" probe. [...]
>>> One might also imagine cases where it could work the other way.
>>>
>>> We could solve this by parametrizing: adding a sequence parameter to
>>> "probe begin(N)" (and "end(M)"), and sorting them.  Easy to
>>> implement. 
>> This is a nice idea -- if you make the default priority zero for those
>> who don't specify it, then things can "just work".  Users can write an
>> unparameterized 'begin' as usual, and the tapset writer can initialize
>> in a 'begin(-1)' -- or 'begin(-2^63)' if paranoia kicks in...
> 
> This is now implemented -- you can give a numeric parameter to begin/end
> probes, and they will execute in increasing order.  The sequence number
> if left out is effectively zero.

Thanks Josh!  I just tried your implementation and it works great.  Just what I needed.

> 
> We should adopt a convention for tapset writers to use these fields.  My
> suggestion: when it doesn't matter, just use begin/end without a
> parameter; to run early, use less than -1000; and to run late use
> greater than 1000.  This way script writers still have +/- 1000 to play
> with locally.

I agree.

Thanks,
Mike

Re: Order "begin" probes are run

[Frank Ch. Eigler]

Hi -

On Thu, Dec 07, 2006 at 06:52:02PM -0800, Stone, Joshua I wrote:
> [...] This is now implemented [...]

Thank you, nice work.

The change to unary number parsing is probably suspect though, in that
it introduces parsing errors or ambiguities.  I thought we had a test
for some instances of this problem.  If there is any doubt, I would
suggest rolling this back, and let the probe point parser accept an
explicit negation operator as an optional prefix. 

- FChE

RE: Order "begin" probes are run

[Stone, Joshua I]

On Wednesday, November 29, 2006 1:57 PM, Stone, Joshua I wrote:
> On Wednesday, November 29, 2006 1:23 PM, Frank Ch. Eigler wrote:
>> Mike Mason <mmlnx@us.ibm.com> writes:
>>> Seems reasonable to expect tapset "begin" probes to always run
>>> before a script's "begin" probe. [...]
>> 
>> One might also imagine cases where it could work the other way.
>> 
>> We could solve this by parametrizing: adding a sequence parameter to
>> "probe begin(N)" (and "end(M)"), and sorting them.  Easy to
>> implement. 
> 
> This is a nice idea -- if you make the default priority zero for those
> who don't specify it, then things can "just work".  Users can write an
> unparameterized 'begin' as usual, and the tapset writer can initialize
> in a 'begin(-1)' -- or 'begin(-2^63)' if paranoia kicks in...

This is now implemented -- you can give a numeric parameter to begin/end
probes, and they will execute in increasing order.  The sequence number
if left out is effectively zero.

We should adopt a convention for tapset writers to use these fields.  My
suggestion: when it doesn't matter, just use begin/end without a
parameter; to run early, use less than -1000; and to run late use
greater than 1000.  This way script writers still have +/- 1000 to play
with locally.


Josh

RE: Order "begin" probes are run

[Stone, Joshua I]

On Wednesday, November 29, 2006 1:23 PM, Frank Ch. Eigler wrote:
> Mike Mason <mmlnx@us.ibm.com> writes:
>> Seems reasonable to expect tapset "begin" probes to always run
>> before a script's "begin" probe. [...]
> 
> One might also imagine cases where it could work the other way.
> 
> We could solve this by parametrizing: adding a sequence parameter to
> "probe begin(N)" (and "end(M)"), and sorting them.  Easy to implement.

This is a nice idea -- if you make the default priority zero for those
who don't specify it, then things can "just work".  Users can write an
unparameterized 'begin' as usual, and the tapset writer can initialize
in a 'begin(-1)' -- or 'begin(-2^63)' if paranoia kicks in...

Josh