[gdb/testsuite] Fix index-cache.exp with cc-with-{gdb-index,debug-names}

Message ID a770cb7f-2be6-3e48-58ce-f90773f0adb2@suse.de
State New
Headers show
Series
  • [gdb/testsuite] Fix index-cache.exp with cc-with-{gdb-index,debug-names}
Related show

Commit Message

Tom de Vries May 4, 2019, 8:35 a.m.
[ was: Re: [PATCH][gdb/testsuite] Fix index-cache.exp with
CC_WITH_TWEAKS_FLAGS=-i ]

On 03-05-19 23:17, Simon Marchi wrote:
> On 2019-05-03 6:43 a.m., Tom de Vries wrote:

>> Hi,

>>

>> When running gdb.base/index-cache.exp with target board cc-with-tweaks with

>> CC_WITH_TWEAKS_FLAGS set to "-i", we run into:

>> ...

>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: at least one file \

>>       was created

>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there

>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: check index-cache stats

>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: check index-cache stats

>> ...

>>

>> The problem is that the target board makes sure that the generated executable

>> contains a .gdb_index section, while the test assumes that the executable

>> doesn't contain this section.

>>

>> Fix this by removing the .gdb_index section from the generated executable.

>>

>> Tested on x86_64-linux with native and CC_WITH_TWEAKS_FLAGS=-i config.

>>

>> OK for trunk?

>>

>> Thanks,

>> - Tom

> 

> Hi Tom,

> 

> I would slightly prefer that instead of doing this, we would notice that that file

> already has an index (in the form of .gdb_index or .debug_names), and adjust our

> expectations in the test.

> 

> In other words, we currently assert that loading the file in GDB will produce some

> files in the cache.  However, if we know that the file already has an index, we

> should verify that no file was produced, as this is the behavior we expect when

> loading a file which already has an index.

> 

> Stripping the index makes the test pass, but it just goes back to testing the same

> thing as with the default board file.  Adjusting our expectation to the presence

> of an index makes the test cover a different use case.


I've implemented this approach, attached below.

OK for trunk?

Thanks,
- Tom

Comments

Simon Marchi May 4, 2019, 4:28 p.m. | #1
On 2019-05-04 4:35 a.m., Tom de Vries wrote:
> [ was: Re: [PATCH][gdb/testsuite] Fix index-cache.exp with

> CC_WITH_TWEAKS_FLAGS=-i ]

> 

> On 03-05-19 23:17, Simon Marchi wrote:

>> On 2019-05-03 6:43 a.m., Tom de Vries wrote:

>>> Hi,

>>>

>>> When running gdb.base/index-cache.exp with target board cc-with-tweaks with

>>> CC_WITH_TWEAKS_FLAGS set to "-i", we run into:

>>> ...

>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: at least one file \

>>>       was created

>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there

>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: check index-cache stats

>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: check index-cache stats

>>> ...

>>>

>>> The problem is that the target board makes sure that the generated executable

>>> contains a .gdb_index section, while the test assumes that the executable

>>> doesn't contain this section.

>>>

>>> Fix this by removing the .gdb_index section from the generated executable.

>>>

>>> Tested on x86_64-linux with native and CC_WITH_TWEAKS_FLAGS=-i config.

>>>

>>> OK for trunk?

>>>

>>> Thanks,

>>> - Tom

>>

>> Hi Tom,

>>

>> I would slightly prefer that instead of doing this, we would notice that that file

>> already has an index (in the form of .gdb_index or .debug_names), and adjust our

>> expectations in the test.

>>

>> In other words, we currently assert that loading the file in GDB will produce some

>> files in the cache.  However, if we know that the file already has an index, we

>> should verify that no file was produced, as this is the behavior we expect when

>> loading a file which already has an index.

>>

>> Stripping the index makes the test pass, but it just goes back to testing the same

>> thing as with the default board file.  Adjusting our expectation to the presence

>> of an index makes the test cover a different use case.

> 

> I've implemented this approach, attached below.

> 

> OK for trunk?

> 

> Thanks,

> - Tom

> 


Thanks Tom, this LGTM.

Before pushing, could you just adjust the comments above each proc?  They describe
what the test expects (at a high level), so maybe just add a precision about what is
expected when the binary already has an index.

Simon
Tom de Vries May 6, 2019, 6:43 a.m. | #2
On 04-05-19 18:28, Simon Marchi wrote:
> On 2019-05-04 4:35 a.m., Tom de Vries wrote:

>> [ was: Re: [PATCH][gdb/testsuite] Fix index-cache.exp with

>> CC_WITH_TWEAKS_FLAGS=-i ]

>>

>> On 03-05-19 23:17, Simon Marchi wrote:

>>> On 2019-05-03 6:43 a.m., Tom de Vries wrote:

>>>> Hi,

>>>>

>>>> When running gdb.base/index-cache.exp with target board cc-with-tweaks with

>>>> CC_WITH_TWEAKS_FLAGS set to "-i", we run into:

>>>> ...

>>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: at least one file \

>>>>       was created

>>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there

>>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: check index-cache stats

>>>> FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: check index-cache stats

>>>> ...

>>>>

>>>> The problem is that the target board makes sure that the generated executable

>>>> contains a .gdb_index section, while the test assumes that the executable

>>>> doesn't contain this section.

>>>>

>>>> Fix this by removing the .gdb_index section from the generated executable.

>>>>

>>>> Tested on x86_64-linux with native and CC_WITH_TWEAKS_FLAGS=-i config.

>>>>

>>>> OK for trunk?

>>>>

>>>> Thanks,

>>>> - Tom

>>>

>>> Hi Tom,

>>>

>>> I would slightly prefer that instead of doing this, we would notice that that file

>>> already has an index (in the form of .gdb_index or .debug_names), and adjust our

>>> expectations in the test.

>>>

>>> In other words, we currently assert that loading the file in GDB will produce some

>>> files in the cache.  However, if we know that the file already has an index, we

>>> should verify that no file was produced, as this is the behavior we expect when

>>> loading a file which already has an index.

>>>

>>> Stripping the index makes the test pass, but it just goes back to testing the same

>>> thing as with the default board file.  Adjusting our expectation to the presence

>>> of an index makes the test cover a different use case.

>>

>> I've implemented this approach, attached below.

>>

>> OK for trunk?

>>

>> Thanks,

>> - Tom

>>

> 

> Thanks Tom, this LGTM.

> 

> Before pushing, could you just adjust the comments above each proc?  They describe

> what the test expects (at a high level), so maybe just add a precision about what is

> expected when the binary already has an index.

> 


Done.

Also replaced use of hardcoded "readelf" with result of gdb_find_readelf.

Thanks,
- Tom
[gdb/testsuite] Fix index-cache.exp with cc-with-{gdb-index,debug-names}

In gdb.base/index-cache.exp, handle the case that binfile contains either a
.gdb_index or .debug_names index section.

Tested on x86_64-linux with native, cc-with-gdb-index and cc-with-debug-names.

gdb/testsuite/ChangeLog:

2019-05-04  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (exec_has_index_section): New proc.
	* gdb.base/index-cache.exp: Handle case that binfile contains an index
	section.

---
 gdb/testsuite/gdb.base/index-cache.exp | 39 +++++++++++++++++++++++++++-------
 gdb/testsuite/lib/gdb.exp              | 12 +++++++++++
 2 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/gdb/testsuite/gdb.base/index-cache.exp b/gdb/testsuite/gdb.base/index-cache.exp
index 4e583abf01..5baba84360 100644
--- a/gdb/testsuite/gdb.base/index-cache.exp
+++ b/gdb/testsuite/gdb.base/index-cache.exp
@@ -22,6 +22,8 @@ if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
     return
 }
 
+set has_index_section [exec_has_index_section $binfile]
+
 # List the files in DIR on the host (where GDB-under-test runs).
 # Return a list of two elements:
 #   - 0 on success, -1 on failure
@@ -122,10 +124,12 @@ proc_with_prefix test_cache_disabled { cache_dir } {
     }
 }
 
-# Test with the cache enabled, we expect to have exactly one file created.
+# Test with the cache enabled, we expect to have:
+# - exactly one file created, in case of no index section
+# - no file created, in case of an index section
 
 proc_with_prefix test_cache_enabled_miss { cache_dir } {
-    global testfile
+    global testfile has_index_section
 
     lassign [ls_host $cache_dir] ret files_before
 
@@ -133,7 +137,11 @@ proc_with_prefix test_cache_enabled_miss { cache_dir } {
 
 	lassign [ls_host $cache_dir] ret files_after
 	set nfiles_created [expr [llength $files_after] - [llength $files_before]]
-	gdb_assert "$nfiles_created > 0" "at least one file was created"
+	if { $has_index_section } {
+	    gdb_assert "$nfiles_created == 0" "no file was created"
+	} else {
+	    gdb_assert "$nfiles_created > 0" "at least one file was created"
+	}
 
 	set build_id [get_build_id  [standard_output_file ${testfile}]]
 	if { $build_id == "" } {
@@ -143,19 +151,30 @@ proc_with_prefix test_cache_enabled_miss { cache_dir } {
 
 	set expected_created_file [list "${build_id}.gdb-index"]
 	set found_idx [lsearch -exact $files_after $expected_created_file]
-	gdb_assert "$found_idx >= 0" "expected file is there"
+	if { $has_index_section } {
+	    gdb_assert "$found_idx == -1" "no index cache file generated"
+	} else {
+	    gdb_assert "$found_idx >= 0" "expected file is there"
+	}
 
 	remote_exec host rm "-f $cache_dir/$expected_created_file"
 
-	check_cache_stats 0 1
+	if { $has_index_section } {
+	    check_cache_stats 0 0
+	} else {
+	    check_cache_stats 0 1
+	}
     }
 }
 
 
-# Test with the cache enabled, this time we should have one file (the
-# same), but one cache read hit.
+# Test with the cache enabled, this time we should have:
+# - one file (the same), but one cache read hit, in case of no index section
+# - no file, no cache hit, in case an an index section
 
 proc_with_prefix test_cache_enabled_hit { cache_dir } {
+    global has_index_section
+
     # Just to populate the cache.
     run_test_with_flags $cache_dir on {}
 
@@ -166,7 +185,11 @@ proc_with_prefix test_cache_enabled_hit { cache_dir } {
 	set nfiles_created [expr [llength $files_after] - [llength $files_before]]
 	gdb_assert "$nfiles_created == 0" "no files were created"
 
-	check_cache_stats 1 0
+	if { $has_index_section } {
+	    check_cache_stats 0 0
+	} else {
+	    check_cache_stats 1 0
+	}
     }
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 57866daa11..8dea857d5e 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5107,6 +5107,18 @@ proc rerun_to_main {} {
   }
 }
 
+# Return true if EXECUTABLE contains a .gdb_index or .debug_names index section.
+
+proc exec_has_index_section { executable } {
+    set readelf_program [gdb_find_readelf]
+    set res [catch {exec $readelf_program -S $executable \
+			| grep -E "\.gdb_index|\.debug_names" }]
+    if { $res == 0 } {
+	return 1
+    }
+    return 0
+}
+
 # Return true if a test should be skipped due to lack of floating
 # point support or GDB can't fetch the contents from floating point
 # registers.

Patch

[gdb/testsuite] Fix index-cache.exp with cc-with-{gdb-index,debug-names}

In gdb.base/index-cache.exp, handle the case that binfile contains either a
.gdb_index or .debug_names index section.

Tested on x86_64-linux with native, cc-with-gdb-index and cc-with-debug-names.

gdb/testsuite/ChangeLog:

2019-05-04  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (exec_has_index_section): New proc.
	* gdb.base/index-cache.exp: Handle case that binfile contains an index
	section.

---
 gdb/testsuite/gdb.base/index-cache.exp | 30 +++++++++++++++++++++++++-----
 gdb/testsuite/lib/gdb.exp              | 11 +++++++++++
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/index-cache.exp b/gdb/testsuite/gdb.base/index-cache.exp
index 4e583abf01..f5e1e4a850 100644
--- a/gdb/testsuite/gdb.base/index-cache.exp
+++ b/gdb/testsuite/gdb.base/index-cache.exp
@@ -22,6 +22,8 @@  if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
     return
 }
 
+set has_index_section [exec_has_index_section $binfile]
+
 # List the files in DIR on the host (where GDB-under-test runs).
 # Return a list of two elements:
 #   - 0 on success, -1 on failure
@@ -125,7 +127,7 @@  proc_with_prefix test_cache_disabled { cache_dir } {
 # Test with the cache enabled, we expect to have exactly one file created.
 
 proc_with_prefix test_cache_enabled_miss { cache_dir } {
-    global testfile
+    global testfile has_index_section
 
     lassign [ls_host $cache_dir] ret files_before
 
@@ -133,7 +135,11 @@  proc_with_prefix test_cache_enabled_miss { cache_dir } {
 
 	lassign [ls_host $cache_dir] ret files_after
 	set nfiles_created [expr [llength $files_after] - [llength $files_before]]
-	gdb_assert "$nfiles_created > 0" "at least one file was created"
+	if { $has_index_section } {
+	    gdb_assert "$nfiles_created == 0" "no file was created"
+	} else {
+	    gdb_assert "$nfiles_created > 0" "at least one file was created"
+	}
 
 	set build_id [get_build_id  [standard_output_file ${testfile}]]
 	if { $build_id == "" } {
@@ -143,11 +149,19 @@  proc_with_prefix test_cache_enabled_miss { cache_dir } {
 
 	set expected_created_file [list "${build_id}.gdb-index"]
 	set found_idx [lsearch -exact $files_after $expected_created_file]
-	gdb_assert "$found_idx >= 0" "expected file is there"
+	if { $has_index_section } {
+	    gdb_assert "$found_idx == -1" "no index cache file generated"
+	} else {
+	    gdb_assert "$found_idx >= 0" "expected file is there"
+	}
 
 	remote_exec host rm "-f $cache_dir/$expected_created_file"
 
-	check_cache_stats 0 1
+	if { $has_index_section } {
+	    check_cache_stats 0 0
+	} else {
+	    check_cache_stats 0 1
+	}
     }
 }
 
@@ -156,6 +170,8 @@  proc_with_prefix test_cache_enabled_miss { cache_dir } {
 # same), but one cache read hit.
 
 proc_with_prefix test_cache_enabled_hit { cache_dir } {
+    global has_index_section
+
     # Just to populate the cache.
     run_test_with_flags $cache_dir on {}
 
@@ -166,7 +182,11 @@  proc_with_prefix test_cache_enabled_hit { cache_dir } {
 	set nfiles_created [expr [llength $files_after] - [llength $files_before]]
 	gdb_assert "$nfiles_created == 0" "no files were created"
 
-	check_cache_stats 1 0
+	if { $has_index_section } {
+	    check_cache_stats 0 0
+	} else {
+	    check_cache_stats 1 0
+	}
     }
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 57866daa11..84c54062c8 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5107,6 +5107,17 @@  proc rerun_to_main {} {
   }
 }
 
+# Return true if EXECUTABLE contains a .gdb_index or .debug_names index section.
+
+proc exec_has_index_section { executable } {
+    set res [catch {exec readelf -S $executable \
+			| grep -E "\.gdb_index|\.debug_names" }]
+    if { $res == 0 } {
+	return 1
+    }
+    return 0
+}
+
 # Return true if a test should be skipped due to lack of floating
 # point support or GDB can't fetch the contents from floating point
 # registers.