Re: Updated patch adding line number enumeration support to statement probe

[BR Chrisman <brchrisman@gmail.com>]

On Tue, Jun 3, 2014 at 11:44 AM, Jonathan Lebon <jlebon@redhat.com> wrote:
> Hi Brian,
>
>
>> I'll verify my mailing before sending up the next patch.  I've had
>> issues with this before, my apologies.
>
> No worries.
>
>> I cleaned up all the RANGE stuff and will include in another patch.
>
> Great!
>
>> > You seed the linenos vector with two '0' elements, but then never add
>> > the parsed linenos for ABSOLUTE or RELATIVE linenos. So stap thinks that
>> > the user entered lineno 0. I suspect that's what's causing all the failures
>> > in statement.exp.
>>
>> The seed values were intended to mimic the removed line:
>> linenos[0] = linenos[1] = 0;
>>
>> I tried testing with those seeds removed, which failed as well, but
>> I'm confused about how/where ABSOLUTE/RELATIVE specifications are
>> parsed.
>
> The lines that were removed which took care of this were these:
>
> -   dash_pos = spec.find('-', line_pos + 1);
> -   if (dash_pos == string::npos)
> -     linenos[0] = linenos[1] = lex_cast<int>(spec.substr(line_pos + 1));

Ahh yes.. *that* one..


>
>> The two statement.exp test cases which are failing are:
>> FAIL: statement (ENUMERATED and RANGE - single func - expected 3 probes, got
>> 0)
>> FAIL: statement (ENUMERATED and RANGE - wild func - expected 3 probes, got 0)
>> ...
>>                 ===  Summary ===
>> # of expected passes            32
>> # of unexpected failures        2
>
> Strange, I had the following failures when applying your patch:
>
> Running /home/yyz/jlebon/codebase/systemtap/systemtap/testsuite/systemtap.base/statement.exp ...
> FAIL: statement (ABSOLUTE - single func - expected 1 probes, got 0)
> FAIL: statement (ABSOLUTE - wild func - expected 1 probes, got 0)
> FAIL: statement (RELATIVE - single func - expected 1 probes, got 0)
> FAIL: statement (RELATIVE - wild func - expected 2 probes, got 1)
> FAIL: statement (ABSOLUTE - error for no lines - single func - no semantic error)

huh.. well.. if the line I goofed above cleans this up, then it should
be all good I suppose.  My internal code tests work for the
multi-range.

>
>> > For parsing the spec, I think it would be easier to tokenize on commas,
>> > and then check the resulting tokens one by one for either a single lineno
>> > or a range (maybe even factor that part out into its own function). There's
>> > tokenize() in util.cxx which is useful for this.
>>
>> I noticed that I misinterpreted what the code was doing with the range
>> before.
>> This patch simply translates it into an enumeration.
>> Even using binary_search on an enumeration, a search of a large range
>> would be inefficient.
>
> So what I meant here was that it might be easier to parse the a,b-c,d,e,...
> by doing something like this (pseudo-code):
>
> vector<string> tokens;
> tokenize("a,b-c,d,e", tokens, ","); // available from util.h
> for (token in tokens) {
>   if token is single number
>     add single number to linenos vector
>   if token is a range
>     for each number in range
>        add number to linenos vector
> }

Yes, this makes much more sense.

>
> Regarding binary searching, I think that's OK. It's still pretty
> fast. :)
>
>> I'll have to reformulate this.  I'm not sure it helps to go as far as
>> interval tree stuff, but tracking ranges properly, rather than
>> smashing them into enumerations, makes sense to me.
>
>> Yeah, it seems like we need to check against a series of items which
>> could be either individual line numbers or ranges.  This will also
>> solve the output case as we can just parrot out the items.
>> I kind-of see where this can change.  Enumerating the range explicitly
>> was my gut response, but it could be a surprise performance reduction
>> for a large range in a large file of source.
>> I'll try to get this all cleaned up in the next day or two.
>
> I think it would be easier to keep it simple and let the vector just hold
> the individual linenos (like you originally designed). Since we'll need to
> sort the final vector after all the parsing is done anyway (for later
> binary searching if necessary), we can easily print it out in the
> "a,b-c,d,e,..." form by going through the vector and detecting jumps >1.

... makes sense... is there a 'no end of line' clog stream?
I'd like to output piecemeal here, so I'd need a stream that doesn't
automatically endl or accumulate the whole thing into a string.

>
>> thanks for your feedback,
>> Brian
>
> Looking forward to your next patches!
>
>
> Cheers,
>
> Jonathan