diff mbox

Testsuite: Ensure changing directory does not break the log file

Message ID CB2D111D-BDC5-49BA-B3D2-E9D96E3FABDC@arm.com
State New
Headers show

Commit Message

Alan Hayward Feb. 22, 2019, 12:36 p.m. UTC 
> On 21 Feb 2019, at 22:40, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Alan" == Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Alan> get_compiler_info switches to a new log file before checking the compiler
> Alan> to ensure the checks are not logged.
> 
> Haha, that seems pretty gross.

It was quite a fun issue to track down :)

> 
> Alan> The simpler and safer solution is to override the builtin cd function. The
> Alan> new function checks the current log file and if the path is relative, then
> Alan> it resets the logging using an absolute path. Finally it calls the builtin
> Alan> cd.  This ensures get_compiler_info (and any other code) can correctly
> Alan> backup and restore the current log file.
> 
> Makes sense to me.
> 
> Alan> +    if { [regexp "^-a \[^/\]" $saved_log match] } {
> 
> It may be better here to just extract the file name and either use "file normalize"
> (could be done unconditionally) or look at it with "file pathtype".
> See https://www.tcl.tk/man/tcl8.4/TclCmd/file.htm
> 
> Tom

Re-written using normalize and I also made the checking better.  With this version
it will be safe regardless of the arguments used for log_file.

This version ok?

Comments

Pedro Alves Feb. 22, 2019, 2:21 p.m. UTC | #1 
On 02/22/2019 12:36 PM, Alan Hayward wrote:
> +# Safe version of cd that ensures the log file is not stopped.

This comment seems unclear to me, if you don't consider the email context.

What does it mean to "stop a file", for instance?

How about:

   # Override the 'cd' builtin with a version that ensures that the
   # log file keeps pointing at the same file.  We need this because
   # unfortunately the path to the log file is recorded using an
   # relative path name, and, we sometimes need to close/reopen the log
   # after changing the current directory.  See get_compiler_info.

> +
> +rename cd builtin_cd
> +
> +proc cd { dir } {

Thanks,
Pedro Alves
Alan Hayward Feb. 22, 2019, 5:10 p.m. UTC | #2 
> On 22 Feb 2019, at 14:21, Pedro Alves <palves@redhat.com> wrote:

> 

> On 02/22/2019 12:36 PM, Alan Hayward wrote:

>> +# Safe version of cd that ensures the log file is not stopped.

> 

> This comment seems unclear to me, if you don't consider the email context.

> 

> What does it mean to "stop a file", for instance?

> 

> How about:

> 

>   # Override the 'cd' builtin with a version that ensures that the

>   # log file keeps pointing at the same file.  We need this because

>   # unfortunately the path to the log file is recorded using an

>   # relative path name, and, we sometimes need to close/reopen the log

>   # after changing the current directory.  See get_compiler_info.

> 


Ok, I’m happy with switching to that.  I wanted to keep the comment a
little more generic, but agreed it makes more sense this way.


Alan.
diff mbox

Patch

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index d05854329d..f052bd2f3d 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -6284,5 +6284,37 @@  proc gdb_define_cmd {command command_list} {
     }
 }

+# Safe version of cd that ensures the log file is not stopped.
+
+rename cd builtin_cd
+
+proc cd { dir } {
+
+    # Get the existing log file flags.
+    set log_file_info [log_file -info]
+
+    # Split the flags into args and file name.
+    set log_file_flags ""
+    set log_file_file ""
+    foreach arg [ split "$log_file_info" " "] {
+       if [string match "-*" $arg] {
+           lappend log_file_flags $arg
+       } else {
+           lappend log_file_file $arg
+       }
+    }
+
+    # If there was an existing file, ensure it is an absolute path, and then
+    # reset logging.
+    if { $log_file_file != "" } {
+       set log_file_file [file normalize $log_file_file]
+       log_file
+       log_file $log_file_flags "$log_file_file"
+    }
+
+    # Call the builtin version of cd.
+    builtin_cd $dir
+}
+
 # Always load compatibility stuff.
 load_lib future.exp