This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: BZ 6701 - Improve error messages (patch)
- From: fche at redhat dot com (Frank Ch. Eigler)
- To: rarora at redhat dot com
- Cc: systemtap at sources dot redhat dot com
- Date: Wed, 01 Oct 2008 10:01:11 -0400
- Subject: Re: BZ 6701 - Improve error messages (patch)
- References: <48E29E22.4070708@redhat.com>
Hi, Rajan -
> This is a proposed patch for the Bugzilla bug 6701 [...]
Thanks, nice work!
> # stap -ve 'probe begin { log("hello world") exit () }'
> Pass 1: parsed user script and 45 library script(s) in 150usr/10sys/172real ms.
> semantic error: unresolved arity-0 function: identifier 'exxit' at <input>:1:34
> source: probe begin { log("hello world") exxit () }
> ^
Please check the rendering heuristics for the case of the inputs
containing tabs and for lines perhaps too long to draw in their
entirety.
> + //Do file_contents exist? If not, error is in tapset!
> + if (e.tok1 && user_file->file_contents.size() != 0)
> + {
> + errormsg.str ("");
> + print_error_source (errormsg, user_file->file_contents, e.tok1);
> + cerr << errormsg.str();
> + }
For this and similar cases, is there some reason for the errormsg
object? Would this not work just as well?
print_error_source (cerr, user_file->file_contents, e.tok1);
> +systemtap_session::print_error_source (std::stringstream& message,
> + std::string& file_contents, const token* tok)
> +{
> [...]
> + message << "\tsource: " << file_contents.substr (start_pos, end_pos-start_pos-1) << endl;
> + message << "\t ";
> + message.fill (' ');
> + message.width (col);
> + message << "^" << endl;
Yeah, this won't work right if the input contains tabs. Instead of
the .fill()/.width() way of lining up with the input, you could try
looping over the contents of file_contents[start_pos .. end_pos]. You
could copy each isspace(char) and emit a ' ' otherwise.
> +void
> +lexer::get_input_contents (std::string& file_contents)
> +{
> + unsigned i;
> + for (i=0; i<input_contents.size(); i++)
> + file_contents += input_contents[i];
> +}
Could you look into making that input_contents[] widget into a plain
std::string? That should turn this into a simple assignment ...
or rather a "return input_contents;" - the function might as well
return a std::string instead of a value by reference.
> int
> lexer::input_peek (unsigned n)
> [...]
> + //Assuming tapsets are error-free, only user's script contents are fetched
> + if (!strstr (input_name.c_str(),"tapset/"))
> + input.get_input_contents (f->file_contents);
Nothing's error free :-) ... except my wife's mother. Drop this
assumption - we can have semantic errors that occur with e.g. $context
variables or probe points that exist only on a ragtag collection of
kernel versions.
> +++ b/testsuite/parseko/source_context.stp
> @@ -0,0 +1,11 @@
> +global count_test
> +probe timer.ms(123)
> +{
> +printf("%d loops have executed\n",count_test)
> +count_test ++
> +iffff(count_test == 5)
> +{
> +priiintf("Done execution 5 times and about to exit. . .\n")
> + eeexit ()
> +}
> +}
OK (though one error per file might be more assertive).
- FChE