Re: [PATCH 2/2] [gdb/testsuite] Fix host_file_normalize_mingw

[Tom de Vries <tdevries@suse.de>]

On 8/29/25 21:47, Pedro Alves wrote:
> Hi Tom,
> 
> On 2025-08-27 14:14, Tom de Vries wrote:
> 
>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index b7961eb30dc..4b0249db70b 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -2381,9 +2381,9 @@ proc host_file_normalize_mingw {filename unix_to_win} {
>>   	if {[string equal -length $mount_len $unix_filename $filename]} {
>>   	    if {[string length $filename] == $mount_len} {
>>   		return "$win_filename/"
>> -	    } elseif {[string index $filename $mount_len] eq "/"} {
>> +	    } elseif {[string index $filename [expr $mount_len - 1]] eq "/"} {
>>   		set rest [string range $filename $mount_len end]
>> -		return "$win_filename$rest"
>> +		return "$win_filename/$rest"
>>   	    }
>>   	}
>>       }
> 
> 
> Unfortunately, this breaks things for me.
> I test on a directory under c:/, and the testsuite broke very visibly.  :-)
> 
> For example, the patch makes us mishandle this:
> 
>   set f3 "/c/foo/bar"
>   set f4 "C:/foo/bar"
>   gdb_assert { [host_file_normalize_mingw $f3 $unix_to_win] == $f4 }
> 
> Instead of turning $f3 into:
>     C:/foo/bar
> it turns it into:
>     C:/msys64/c/foo/bar
> 
> Notice that the "/" mount point is the only one that ends in "/".  This
> is even if you try to create one explicitly with a trailing /.  On MSYS2:
> 
>   $ mount c:/foo /foo/
>   mount: warning - /foo/ does not exist.
>   $ mount
>   C:/foo on /foo type ntfs (binary,user)
>   ...
> 
> So I think we need to instead special case the "/" mount point.
> 
> And then... while playing with this, I noticed I had done something strange with this case:
> 
>    	    if {[string length $filename] == $mount_len} {
>    		return "$win_filename/"
> 
> The intent was to append the slash when the mount is a drive letter, like 'cygpath -ma' does:
> 
>   $ cygpath -ma /c
>   C:/
> 
> Other cases do not get a trailing slash:
> 
>   $ cygpath -ma /c/foo
>   C:/foo
> 
> I think this is because on Windows, every drive letter has a current directory, and really "C:" means
> "current directory of drive letter C:", not "root of C:".  Resolving it to "C:/" makes it unambiguous.
> 
> However, I mishandled that, and made the code append the slash whenever the input filename matches
> a mount exactly, any mount.
> 
> And then I noticed that TCL's "file normalize" on Linux always removes the trailing slash, and since
> host_file_normalize is an abstraction for it, I thought host_file_normalize_mingw should do the same.
> Likewise for duplicate slashes, "file normalize" gets rid of them.
> 
> I was going to give you a suggestion for the root mount fix, and handle the slashes things in a follow
> up patch to yours, but fixing the slashes details changes how to address the issue with handling the
> root dir, so I ended up doing it all in one patch.  See below.
> 
> I changed how one adds tests to your new testcase to a way that I think is much easier to write, read,
> and makes gdb.sum results also have some sense.  It also prints some info to gdb.log that helped me
> understand what went wrong when a test FAILs.
> 
> The "/foo" test I added has the same effect as yours, just with a shorter and simpler filename, that
> fits better with the other new tests I added.  What matters is that this case is a file/directory under
> a mount point, and I made the testcase test that for every mount point (root, drive letter, other).
> 
> I absolutely love the idea of a testcase for this.  It helps so much!  Thanks a lot for starting that.
> 
> WDYT?  Does this version work for you?  I smoke tested it here with a few of the testcases that required
> tweaking in the patch that added host_file_normalize like gdb.base/source-dir.exp and gdb.base/fullname.exp
> and they still passed.
> 

Hi Pedro,

I ran the test-case on both x86_64-linux and msys2-ucrt64, and it passed 
in both cases, so I think it should be fine.

Feel free to commit this together with the first patch, or perhaps to 
combined into one patch.

Given that the second patch now changes the setup of the test-case, I 
think squashing makes more sense.

Thanks,
- Tom

>  From de4e47323d9e7df1b09cd635bfce902b00b3371b Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 29 Aug 2025 20:05:48 +0100
> Subject: [PATCH] Fix host_file_normalize_mingw
> 
> Change-Id: I852a8662f0cb8b0ee4e683e9b157618cf6955477
> ---
>   .../gdb.testsuite/mount-point-map.exp         | 33 ++++++++++++++++---
>   gdb/testsuite/lib/gdb.exp                     | 26 +++++++++++++--
>   2 files changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.testsuite/mount-point-map.exp b/gdb/testsuite/gdb.testsuite/mount-point-map.exp
> index d4c8bfdeff6..9e462bb63e9 100644
> --- a/gdb/testsuite/gdb.testsuite/mount-point-map.exp
> +++ b/gdb/testsuite/gdb.testsuite/mount-point-map.exp
> @@ -18,9 +18,32 @@ set unix_to_win {
>       / C:/msys64
>   }
>   
> -set d1 "/"
> -set d2 "C:/msys64/"
> -gdb_assert { [host_file_normalize_mingw $d1 $unix_to_win] == $d2 }
> +# Test that FROM is normalized to TO.
>   
> -set d3 "C:/msys64"
> -gdb_assert { [host_file_normalize_mingw $d3 $unix_to_win] == $d3 }
> +proc test {from to} {
> +    set got [host_file_normalize_mingw $from $::unix_to_win]
> +    verbose -log "input:    $from"
> +    verbose -log "expected: $to"
> +    verbose -log "got:      $got"
> +    gdb_assert {$got == $to} $from
> +}
> +
> +# Drive letters always get a '/' suffix, other Windows file names do
> +# not.
> +test "/" "C:/msys64"
> +test "/c" "C:/"
> +test "/bin" "C:/msys64/usr/bin"
> +
> +# A file name that already starts with a drive letter.
> +test "C:/msys64" "C:/msys64"
> +
> +# A subdir/subfile under each mount.
> +test "/foo" "C:/msys64/foo"
> +test "/c/foo" "C:/foo"
> +test "/bin/foo" "C:/msys64/usr/bin/foo"
> +
> +# Test slash normalization.
> +test "//" "C:/msys64"
> +test "/c///foo//bar//" "C:/foo/bar"
> +# We don't currently handle UNC paths.
> +test "//server///" "C:/msys64/server"
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 18e449ad8a0..17eb00a10f3 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -2374,12 +2374,34 @@ proc host_file_normalize_mingw {filename unix_to_win} {
>   	return $filename
>       }
>   
> +    # Collapse all repeated forward slashes.
> +    set filename [regsub -all {//+} $filename {/}]
> +
> +    # Strip trailing slash, except for root.
> +    if {$filename ne "/" && [string match */ $filename]} {
> +	set filename [string range $filename 0 end-1]
> +    }
> +
>       foreach {unix_filename win_filename} $unix_to_win {
>   	set mount_len [string length $unix_filename]
>   	if {[string equal -length $mount_len $unix_filename $filename]} {
> -	    if {[string length $filename] == $mount_len} {
> -		return "$win_filename/"
> +	    if {$unix_filename eq "/"} {
> +		if {$filename eq "/"} {
> +		    return "$win_filename"
> +		} else {
> +		    return "$win_filename$filename"
> +		}
> +	    } elseif {[string length $filename] == $mount_len} {
> +		# Like "cygpath -ma" if the file name resolves to a
> +		# drive letter, append a slash, to make it unambiguous
> +		# that we resolved to the root of the drive and not
> +		# the drive's current directory.
> +		if {[string match {[A-Za-z]:} $win_filename]} {
> +		    return "$win_filename/"
> +		} else {
> +		    return "$win_filename"
> +		}
>   	    } elseif {[string index $filename $mount_len] eq "/"} {
>   		set rest [string range $filename $mount_len end]
>   		return "$win_filename$rest"
>