[RFA] Ensure deterministic result order in gdb.ada/info_auto_lang.exp

  standard_ada_testfile, standard_test_file and the explicit
csrcfile assignment in info_auto_lang.exp all gives similar pathnames
prefix for a source, such as
/home/philippe/gdb/git/build_binutils-gdb/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.<something>.

However, the gnat compiler normalizes Ada sources path when compiling.
So, the 'Ada' .o object are referencing a pathname such as
/home/philippe/gdb/git/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb,
while the 'C' .o object still references the not normalized pathname.

As the results of 'info functions | ...' are sorted by pathname first,
the order of the results depends on the comparison between different directories,
leading to results that can change depending on these directories.

=> Ensure the result order is always the same, by normalising the C source file.

Tested by running the testcase in 2 different builds, that without normalize
were giving different results.

Note: such 'set csrcfile' is used in 4 other tests mixing Ada and C.
If deemed better (Joel?), I can factorize building such a csrcfile
and normalizing it in an ada.exp standard_csrcfile_for_ada function.

gdb/testsuite/ChangeLog
2018-12-01  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* gdb.ada/info_auto_lang.exp: Normalize some_c source file.
	Update order of results accordingly.
---
 gdb/testsuite/gdb.ada/info_auto_lang.exp | 38 +++++++++++++-----------
 1 file changed, 21 insertions(+), 17 deletions(-)
  

Thanks

Philippe

On Sat, 2018-12-01 at 14:38 +0100, Philippe Waroquiers wrote:
> standard_ada_testfile, standard_test_file and the explicit
> csrcfile assignment in info_auto_lang.exp all gives similar pathnames
> prefix for a source, such as
> /home/philippe/gdb/git/build_binutils-gdb/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.<something>.
> 
> However, the gnat compiler normalizes Ada sources path when compiling.
> So, the 'Ada' .o object are referencing a pathname such as
> /home/philippe/gdb/git/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb,
> while the 'C' .o object still references the not normalized pathname.
> 
> As the results of 'info functions | ...' are sorted by pathname first,
> the order of the results depends on the comparison between different directories,
> leading to results that can change depending on these directories.
> 
> => Ensure the result order is always the same, by normalising the C source file.
> 
> Tested by running the testcase in 2 different builds, that without normalize
> were giving different results.
> 
> Note: such 'set csrcfile' is used in 4 other tests mixing Ada and C.
> If deemed better (Joel?), I can factorize building such a csrcfile
> and normalizing it in an ada.exp standard_csrcfile_for_ada function.
> 
> gdb/testsuite/ChangeLog
> 2018-12-01  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.ada/info_auto_lang.exp: Normalize some_c source file.
> 	Update order of results accordingly.
> ---
>  gdb/testsuite/gdb.ada/info_auto_lang.exp | 38 +++++++++++++-----------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.ada/info_auto_lang.exp b/gdb/testsuite/gdb.ada/info_auto_lang.exp
> index 4ba79fff42..31d706241a 100644
> --- a/gdb/testsuite/gdb.ada/info_auto_lang.exp
> +++ b/gdb/testsuite/gdb.ada/info_auto_lang.exp
> @@ -24,7 +24,11 @@ load_lib "ada.exp"
>  
>  standard_ada_testfile proc_in_ada
>  set cfile "some_c"
> -set csrcfile ${srcdir}/${subdir}/${testdir}/${cfile}.c
> +# gnat normalizes proc_in_ada source file when compiling.
> +# As the 'info' commands results are sorted by absolute path names, also normalize
> +# the some_c source file to ensure that the 'info' results are always
> +# giving Ada results first.
> +set csrcfile [file normalize ${srcdir}/${subdir}/${testdir}/${cfile}.c]
>  set cobject [standard_output_file ${cfile}.o]
>  
>  if { [gdb_compile "${csrcfile}" "${cobject}" object [list debug]] != "" } {
> @@ -111,41 +115,41 @@ foreach_with_prefix language_choice { "auto" "ada" "c" } {
>  		[multi_line \
>  		     "All functions matching regular expression \"proc_in_\":" \
>  		     "" \
> -		     "File .*some_c.c:" \
> -		     $func_in_c($c_match) \
> -		     "" \
>  		     "File .*proc_in_ada.adb:" \
> -		     $func_in_ada($ada_match)
> +		     $func_in_ada($ada_match) \
> +		     "" \
> +		     "File .*some_c.c:" \
> +		     $func_in_c($c_match)
>  		]
>  
>  	    gdb_test "info types some_type" \
>  		[multi_line \
> -		     "All types matching regular expression \"some_type\":" \
> -		     "" \
> -		     "File .*some_c.c:" \
> -		     $type_in_c($c_match) \
> +		     "All types matching regular expression \"some_type\":"  \
>  		     "" \
>  		     "File .*global_pack.ads:" \
> -		     $type_in_ada($ada_match)
> +		     $type_in_ada($ada_match)\
> +		     "" \
> +		     "File .*some_c.c:" \
> +		     $type_in_c($c_match)
>  		]
>  
>  	    gdb_test "info variables some_struct" \
>  		[multi_line \
>  		     "All variables matching regular expression \"some_struct\":" \
>  		     "" \
> -		     "File .*some_c.c:" \
> -		     $var_in_c($c_match) \
> -		     "" \
>  		     "File .*global_pack.ads:" \
> -		     $var_in_ada($ada_match)
> +		     $var_in_ada($ada_match) \
> +		     "" \
> +		     "File .*some_c.c:" \
> +		     $var_in_c($c_match)
>  		]
>  
>  	    gdb_test "rbreak proc_in_" \
>  		[multi_line \
> -		     "Breakpoint.*file .*some_c.c,.*" \
> -		     $rbreak_func_in_c($c_match) \
>  		     "Breakpoint.*file .*proc_in_ada.adb,.*" \
> -		     $rbreak_func_in_ada($ada_match)
> +		     $rbreak_func_in_ada($ada_match) \
> +		     "Breakpoint.*file .*some_c.c,.*" \
> +		     $rbreak_func_in_c($c_match)
>  		]
>  	    delete_breakpoints
>  	}
  

Adding Joel in CC so he has a chance to see this.

On 2018-12-01 8:38 a.m., Philippe Waroquiers wrote:
> standard_ada_testfile, standard_test_file and the explicit
> csrcfile assignment in info_auto_lang.exp all gives similar pathnames
> prefix for a source, such as
> /home/philippe/gdb/git/build_binutils-gdb/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.<something>.
> 
> However, the gnat compiler normalizes Ada sources path when compiling.
> So, the 'Ada' .o object are referencing a pathname such as
> /home/philippe/gdb/git/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb,
> while the 'C' .o object still references the not normalized pathname.
> 
> As the results of 'info functions | ...' are sorted by pathname first,
> the order of the results depends on the comparison between different directories,
> leading to results that can change depending on these directories.
> 
> => Ensure the result order is always the same, by normalising the C source file.
> 
> Tested by running the testcase in 2 different builds, that without normalize
> were giving different results.
> 
> Note: such 'set csrcfile' is used in 4 other tests mixing Ada and C.
> If deemed better (Joel?), I can factorize building such a csrcfile
> and normalizing it in an ada.exp standard_csrcfile_for_ada function.
> 
> gdb/testsuite/ChangeLog
> 2018-12-01  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.ada/info_auto_lang.exp: Normalize some_c source file.
> 	Update order of results accordingly.

I'd just like to clarify, because I don't think I see the same current behavior as you.  When I
run the test without your patch applied, I see this output, for example:

 83 info functions proc_in_^M
 84 All functions matching regular expression "proc_in_":^M
 85 ^M
 86 File /home/simark/src/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb:^M
 87 17:     procedure proc_in_ada;^M
 88 ^M
 89 File /home/simark/src/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/some_c.c:^M
 90 24:     void proc_in_c(void);^M
 91 (gdb) FAIL: gdb.ada/info_auto_lang.exp: language_choice=auto: frame=0, frame_lang=c: info functions proc_in_

If I understand correctly, in your case, you see the C source file as non-normalized?  Note
that I also build out-of-tree.

Since both paths appear already normalized in my case, the test does fails for me and your patch
would fix it.  I am 99% sure I am fine with your fix, I just want to make sure we are on the same
page.

About the other tests you mention, would it be important to normalize the paths there too, or
you suggest to do it just for consistency?

Simon

Simon
  

On Tue, 2018-12-18 at 23:30 -0500, Simon Marchi wrote:
> Adding Joel in CC so he has a chance to see this.
> 
> On 2018-12-01 8:38 a.m., Philippe Waroquiers wrote:
> > standard_ada_testfile, standard_test_file and the explicit
> > csrcfile assignment in info_auto_lang.exp all gives similar pathnames
> > prefix for a source, such as
> > /home/philippe/gdb/git/build_binutils-gdb/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.<something>.
> > 
> > However, the gnat compiler normalizes Ada sources path when compiling.
> > So, the 'Ada' .o object are referencing a pathname such as
> > /home/philippe/gdb/git/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb,
> > while the 'C' .o object still references the not normalized pathname.
> > 
> > As the results of 'info functions | ...' are sorted by pathname first,
> > the order of the results depends on the comparison between different directories,
> > leading to results that can change depending on these directories.
> > 
> > => Ensure the result order is always the same, by normalising the C source file.
> > 
> > Tested by running the testcase in 2 different builds, that without normalize
> > were giving different results.
> > 
> > Note: such 'set csrcfile' is used in 4 other tests mixing Ada and C.
> > If deemed better (Joel?), I can factorize building such a csrcfile
> > and normalizing it in an ada.exp standard_csrcfile_for_ada function.
> > 
> > gdb/testsuite/ChangeLog
> > 2018-12-01  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> > 
> > 	* gdb.ada/info_auto_lang.exp: Normalize some_c source file.
> > 	Update order of results accordingly.
> 
> I'd just like to clarify, because I don't think I see the same current behavior as you.  When I
> run the test without your patch applied, I see this output, for example:
> 
>  83 info functions proc_in_^M
>  84 All functions matching regular expression "proc_in_":^M
>  85 ^M
>  86 File /home/simark/src/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb:^M
>  87 17:     procedure proc_in_ada;^M
>  88 ^M
>  89 File /home/simark/src/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/some_c.c:^M
>  90 24:     void proc_in_c(void);^M
>  91 (gdb) FAIL: gdb.ada/info_auto_lang.exp: language_choice=auto: frame=0, frame_lang=c: info functions proc_in_
> 
> If I understand correctly, in your case, you see the C source file as non-normalized?  Note
> that I also build out-of-tree.
Effectively. Here is what I have without the patch :
  info functions proc_in_
  All functions matching regular expression "proc_in_":

  File /bd/home/philippe/gdb/git/binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb:
  17:	procedure proc_in_ada;

  File /bd/home/philippe/gdb/git/build_binutils-gdb/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.ada/info_auto_lang/some_c.c:
  24:	void proc_in_c(void);
  (gdb) FAIL: gdb.ada/info_auto_lang.exp: language_choice=auto: frame=0, frame_lang=c: info functions proc_in_

And with the patch:
  info functions proc_in_
  All functions matching regular expression "proc_in_":

  File /bd/home/philippe/gdb/git/info_t/gdb/testsuite/gdb.ada/info_auto_lang/proc_in_ada.adb:
  17:	procedure proc_in_ada;

  File /bd/home/philippe/gdb/git/info_t/gdb/testsuite/gdb.ada/info_auto_lang/some_c.c:
  24:	void proc_in_c(void);
  (gdb) PASS: gdb.ada/info_auto_lang.exp: language_choice=auto: frame=0, frame_lang=c: info functions proc_in_

> 
> Since both paths appear already normalized in my case, the test does fails for me and your patch
> would fix it.  I am 99% sure I am fine with your fix, I just want to make sure we are on the same
> page.
A mystery to me why paths are already normalized in your build ...
What compilation command do you see in the gdb.log for some_c.c ?

> 
> About the other tests you mention, would it be important to normalize the paths there too, or
> you suggest to do it just for consistency?
As far as I know, for the other Ada tests, there is currently no need to normalize.
But we currently have the below:
grep 'set csrcfile' gdb/testsuite/gdb.ada/*exp
gdb/testsuite/gdb.ada/arraydim.exp:set csrcfile ${srcdir}/${subdir}/${testdir}/${cfile}.c
gdb/testsuite/gdb.ada/bp_c_mixed_case.exp:set csrcfile ${srcdir}/${subdir}/${testdir}/${cfile}.c
gdb/testsuite/gdb.ada/bp_c_mixed_case.exp:set csrcfile2 ${srcdir}/${subdir}/${testdir}/${cfile2}.c
gdb/testsuite/gdb.ada/cond_lang.exp:set csrcfile ${srcdir}/${subdir}/${testdir}/${cfile}.c
gdb/testsuite/gdb.ada/info_auto_lang.exp:set csrcfile [file normalize ${srcdir}/${subdir}/${testdir}/${cfile}.c]
gdb/testsuite/gdb.ada/lang_switch.exp:set csrcfile ${srcdir}/${subdir}/${testdir}/${cfile}.c

It looks a reasonable idea to factorize this in standard_csrcfile_for_ada,
and in this function, always normalize cannot harm, IMO.

But we can for sure work without that function, up to Joel to say if he believes it is
better to factorize (in this patch, or in another patch).

Thanks

Philippe
  

> > Since both paths appear already normalized in my case, the test does fails for me and your patch
> > would fix it.  I am 99% sure I am fine with your fix, I just want to make sure we are on the same
> > page.
> A mystery to me why paths are already normalized in your build ...
> What compilation command do you see in the gdb.log for some_c.c ?

I think it depends how you configure your build, and where you do
your testing. That's why Simon asked you about it.

Personally, I build and test out-of-tree, and this is the recommended
way. And the second important element is the fact that I use an absolute
path to the configure script:

    $ cd /path/to/build
    $ /path/to/src/configure [...]
    $ make

When you configure GDB that way, then the testsuite framework will
compile the C files by passing the absolute path to those C files.
For instance on example from my gdb.log file:

    | Executing on host: gcc [...] -c -g [snip snip] /path/to/src/gdb/testsuite/gdb.base/gdb1090.c    (timeout = 300)

However, if you configure using a relative path to the configure script,
things become different. With the same example, but configured using...

    $ cd /path/to/build
    $ ../src/configure [...]

... the path given by the testsuite frame to GCC in order to compile
C files now includes these relative directory adjustments you are
seeing. Eg:

    | Executing on host: gcc [...] -c -g [...] /path/to/bld/gdb/testsuite/../../../../act/gdb-public/gdb/testsuite/gdb.base/gdb1090.c    (timeout = 300)

Whether it must be this way or not and why, I don't know. But I don't
think it's even worth exploring the idea of changing it.
  

On 2018-12-20 00:29, Joel Brobecker wrote:
>> > Since both paths appear already normalized in my case, the test does fails for me and your patch
>> > would fix it.  I am 99% sure I am fine with your fix, I just want to make sure we are on the same
>> > page.
>> A mystery to me why paths are already normalized in your build ...
>> What compilation command do you see in the gdb.log for some_c.c ?
> 
> I think it depends how you configure your build, and where you do
> your testing. That's why Simon asked you about it.
> 
> Personally, I build and test out-of-tree, and this is the recommended
> way. And the second important element is the fact that I use an 
> absolute
> path to the configure script:
> 
>     $ cd /path/to/build
>     $ /path/to/src/configure [...]
>     $ make
> 
> When you configure GDB that way, then the testsuite framework will
> compile the C files by passing the absolute path to those C files.
> For instance on example from my gdb.log file:
> 
>     | Executing on host: gcc [...] -c -g [snip snip]
> /path/to/src/gdb/testsuite/gdb.base/gdb1090.c    (timeout = 300)
> 
> However, if you configure using a relative path to the configure 
> script,
> things become different. With the same example, but configured using...
> 
>     $ cd /path/to/build
>     $ ../src/configure [...]
> 
> ... the path given by the testsuite frame to GCC in order to compile
> C files now includes these relative directory adjustments you are
> seeing. Eg:
> 
>     | Executing on host: gcc [...] -c -g [...]
> /path/to/bld/gdb/testsuite/../../../../act/gdb-public/gdb/testsuite/gdb.base/gdb1090.c
>    (timeout = 300)
> 
> Whether it must be this way or not and why, I don't know. But I don't
> think it's even worth exploring the idea of changing it.

Then, I think it makes sense to normalize the paths in order to have a 
predictable output, regardless of how gdb was configured, as Philippe 
suggested.  Are you also ok with his patch?

Simon
  

Simon/Joel, thanks for the help, I see everyday that I am far to master the gdb
subtleties
:).


On Thu, 2018-12-20 at 00:50 -0500, Simon Marchi wrote:
> On 2018-12-20 00:29, Joel Brobecker wrote:
...
> > However, if you configure using a relative path to the configure 
> > script,
> > things become different. 
Effectively, I am building and testing out-of-tree, but with a relative
configure => I will change to use an absolute configure path.

> Then, I think it makes sense to normalize the paths in order to have a 
> predictable output, regardless of how gdb was configured, as Philippe 
> suggested.  Are you also ok with his patch?

And if ok, do we better do standard_csrcfile_for_ada used for all
such c files used by gdb.ada, or do we limit the normalization to this test ?

Philippe
  

> Simon/Joel, thanks for the help, I see everyday that I am far to
> master the gdb subtleties :).

Please point me in the direction of someone who knows them all.. ;-)

> On Thu, 2018-12-20 at 00:50 -0500, Simon Marchi wrote:
> > On 2018-12-20 00:29, Joel Brobecker wrote:
> ...
> > > However, if you configure using a relative path to the configure 
> > > script,
> > > things become different. 
> Effectively, I am building and testing out-of-tree, but with a relative
> configure => I will change to use an absolute configure path.

I don't think you have to, and from my perspective, I don't see
why it should be a requirement.

> > Then, I think it makes sense to normalize the paths in order to have a 
> > predictable output, regardless of how gdb was configured, as Philippe 
> > suggested.  Are you also ok with his patch?
> 
> And if ok, do we better do standard_csrcfile_for_ada used for all such
> c files used by gdb.ada, or do we limit the normalization to this test
> ?

I don't have a strong opinion. I don't see a compelling reason to
do the normalization systematically at the moment, as perhaps
it gives us more diverse testing coverage. The cost is indeed
those occasional testcases that fail for some but not others.
I don't know if we expect this to happen so often that we need
to make it systematic.

I would personally start with your patch, and adjust later, if it turns
out we have enough testcases facing that issue.

And for the avoidance of doubt, and not hold the patch waiting for me,
your patch looks good to me.
  

On Thu, 2018-12-20 at 11:57 +0400, Joel Brobecker wrote:
> I would personally start with your patch, and adjust later, if it turns
> out we have enough testcases facing that issue.
> 
> And for the avoidance of doubt, and not hold the patch waiting for me,
> your patch looks good to me.
Thanks, patch pushed (with only normalizing in this test, as discussed above).

Philippe