Re: [PATCH 8/8] Tests for validate symbol file using build-id.

[Aleksandar Ristovski <aristovski@qnx.com>]

On 13-04-14 10:18 AM, Jan Kratochvil wrote:
> On Tue, 09 Apr 2013 17:27:45 +0200, Aleksandar Ristovski wrote:
....
>> +#define LIB "./solib-mismatch.so"
>> +
>> +int main(int argc, char *argv[])
>
> GNU Coding Standards:
> 	int main (int argc, char *argv[])

Ok.

>
>
>> +{
>> +  void *h;
>
>> +  int (*foo)(void);
> GNU Coding Standards:
>    int (*foo) (void);

Ok.

>
>
...

> And then you can just:
> 	if (chdir (DIRNAME) != 0)
> and that's all.

Ok. Also, LIB is now communicated the same way.

>
>
...
>> +  h = dlopen(LIB, RTLD_NOW);
>
> GNU Coding Standards:
>    h = dlopen (LIB, RTLD_NOW);
>

Ok.

>
...
> GNU Coding Standards:
>    foo = dlsym (h, "foo"); /* set breakpoint 1 here */
>

Ok.

>
...
> GNU Coding Standards:
>    dlclose (h);
>

Ok.

>
...
>> +  untested "get_compiler_info failed."
>
> Missing:
>        return -1
>
> untested does not return on its own.

Ok.

>
...
>> +# Executeable
>
> typo:
> # Executable

Ok.

>
>
>> +set srcfile ${testfile}.c
>> +set executable ${testfile}
>> +set objfile [standard_output_file ${executable}.o]
>> +set binfile [standard_output_file ${executable}]
>
> stcfile and binfile are already set by standard_testfile.
>
>
> Here should be:
> 	file delete -force -- "${binlibfiledirrun}"
> otherwise the testcase errors out on:
> 	rm -rf gdb.base/solib-mismatch_wd; touch gdb.base/solib-mismatch_wd; runtest gdb.base/solib-mismatch.exp

Ok.

>
>
>> +
>> +file mkdir "${binlibfiledirrun}"
>> +
>> +set exec_opts {}
>> +
>> +if { ![istarget "*-*-nto-*"] } {
>> +  set exec_opts [list debug shlib_load]
>
> Rather lappend.
>

Ok.

>
>> +}
>> +
>> +if { [prepare_for_testing $testfile.exp $executable $srcfile $exec_opts] != 0 } {
>> +  return -1
>> +}
>
> I already wrote:
> Use build_executable here as prepare_for_testing just additionally calls
> clean_restart but you do prepare_for_testing on your own later.

Ok.

>
>
>> +
>> +if { [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilerun}" "${binlibfilerun}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != ""
>> +     || [gdb_gnu_strip_debug "${binlibfilerun}"]
>> +     || [gdb_compile_shlib "${srcdir}/${subdir}/${srclibfilegdb}" "${binlibfilegdb}" [list debug ldflags=-Wl,-soname,${binlibfilebase},--build-id]] != "" } {
>
> -soname is already set by gdb_compile_shlib, why do you set it yourself here?
>

I don't need to. Rearranged.


>
...
>
> Empty line before a comment.

Ok.

>
>> +# Do not auto load shared libraries, the test needs to have control
>> +# over when the relevant output gets printed
>
> Missing final dot.
>
> Indent the comment by two spaces right to align with the code.

Ok.

>
>
>> +  send_gdb "set auto-solib-add off\n"
>
> Already written before:
>
> Never (only in some exceptional cases) use send_gdb, it creates races wrt
> syncing on end of the commands.  Use gdb_test or gdb_test_no_output.
>
> The testcase even does not run for me with send_gdb when I use the "read1"
> reproducer catching such racy testcases behavior from:
> 	reproducer for races of expect incomplete reads
> 	http://sourceware.org/bugzilla/show_bug.cgi?id=12649

Ok.

>
>
>> +
>> +  set bp_location [gdb_get_line_number "set breakpoint 1 here"]
>> +
>> +  gdb_breakpoint ${srcfile}:${bp_location} temporary no-message
>
> I already explained in detail why no-message is inappropriate here.

Ok.

>
>
>> +
>> +  gdb_run_cmd { ${binlibfiledirrun} }
>
> main() no longer uses argv[1] so you can remove this parameter here.

Ok.

>
>
>> +  gdb_test "" "set breakpoint 1 here.*" ""
>
> But all these commands here, specifically these:
>
> set bp_location [gdb_get_line_number "set breakpoint 1 here"]
> gdb_breakpoint ${srcfile}:${bp_location} temporary no-message
> gdb_run_cmd { ${binlibfiledirrun} }
> gdb_test "" "set breakpoint 1 here.*" ""
>
> seem overcomplicated to me, it is enough to use:
>
> if ![runto "${srcfile}:[gdb_get_line_number "set breakpoint 1 here"]"] {
>    return
> }

Ok, thanks.

>
>
>> +  send_gdb "sharedlibrary\n"
>> +  gdb_test "" "" ""
>
> Again never use send_gdb.

Ok.

>
>
>> +
>> +  set nocrlf "\[^\r\n\]*"
>> +  set expected_header "From${nocrlf}To${nocrlf}Syms${nocrlf}Read${nocrlf}Shared${nocrlf}"
>> +  set expected_line "${symsloaded}${nocrlf}${solibfile}"
>> +
>> +  gdb_test "info sharedlibrary ${solibfile}" \
>> +    "${expected_header}\r\n.*${expected_line}.*" \
>> +    "${msg} - Symbols for ${solibfile} loaded: expected '${symsloaded}'"
>
> Those .* around ${expected_line} destroy the whole purpose of ${nocrlf} as
> they can match anything.  To make it really single-line one can use for
> example:

I was aiming at matching ${symsloaded} and ${solibfile} on _the_same_ 
line, but ignore if there are extra lines. I believe this guards against 
e.g.

............. Yes ....... SomeOtherLibraryNotSureWhyMatchedTheRegexp
............. No ......... ${solibfile}

In this case, if 'Yes' is expected, it would not match.

>
>    set footer_line {\(\*\): Shared library is missing debugging information\.}
> +
>      "${expected_header}\r\n${nocrlf}${expected_line}${nocrlf}(?:\r\n$footer_line)?" \
>
>
>> +  return 0
>
> The return value (0) is not used by any caller.  And also just "return" at and
> of proc is redundant, remove it.

Actually, I was going to use it to communicate failures within the 
function - changed accordingly. If something goes wrong in one run of 
solib_matching_test, it will print UNTESTED and continue, I believe this 
would aid diagnostics of the issues.

>
>
...
>> +
>> +
>> +
>
> It is a nitpick but you leave in almost all files trailing empty lines.

Ok. I think I removed them. I didn't think they were evil :-)



Thanks,

Aleksandar