Bug 31527

The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=eac88f3298491fdf2caa0d7dd97a3dde954b8b74

commit eac88f3298491fdf2caa0d7dd97a3dde954b8b74
Author: Zhiqing Xiong <zhiqxion@qti.qualcomm.com>
Date:   Fri Apr 5 09:41:40 2024 +0100

    Add support for Windows network paths to the UNC support in _bfd_real_open().
    
    PR 31527

Hi Zhiqing,

  Thank you for reporting this issue and providing a solution.

  I have applied your patch to the sources.  I am a little bit concerned that the shlwapi.h header file might not be present in all WIN32 development environments - but if necessary we can always add a configure time check to handle it absence.

Cheers
  Nick

Thanks Nick.

does this change will be released in GDB 15.1 ?

(In reply to Zhiqing Xiong from comment #3)

> does this change will be released in GDB 15.1 ?

Yes.  Or maybe 14.3.  I am not sure of the number of the next GDB release...

That's great.

Thanks Nick.

With the addition of this patch I haven't been able to build gas (at least on Windows with the tools the msys2 project provides) due to:

  bfdio.c:(.text+0x169): undefined reference to `__imp_PathIsNetworkPathA'

I can work around this by setting LIBS to "-lshlwapi" based on the documentation for that function at https://learn.microsoft.com/en-us/windows/win32/api/shlwapi/nf-shlwapi-pathisnetworkpatha#requirements but this seems non-ideal. Zhiqing, were you specifying any variables when building with this change? (checking this isn't an issue just with my local configuration)

As an aside, I think this might be a duplicate of Bug 29531, at the very least I was working on a fix for this and the attached patch looks very similar to what I had locally.

Hi Simon

I saw several bugs about gdb not support UNC path without comment, sorry, I didn't know you are working on it. 

I have to upgrade gdb to latest version to support ZSTD compressed Dwarf.
I found this issue. and eagerly hope that the next version could fix this issue.

  My environment is msys64 on Windows as well, could you check if shlwapi.h header file exist. PathIsNetworkPathA definition was there.

  >> C:\msys64\mingw64\include\shlwapi.h 



Install necessary packages on msys64.

  pacman -Syuu
  pacman -S diffutils make texinfo libiconv dejagnu bison git
  pacman -S mingw-w64-x86_64-toolchain

The definition is there (I have the header file), this was a linking issue (and to confirm we are using the same tools to build).

I've looked through the Makefile/config.status files for my build, and what seems to be going on is you're able to build GDB because you're building against the python in that environment, and that requires pulling in the shlwapi library:

  S["PYTHON_LIBS"]="-lpython3.11 -lversion -lshlwapi -lpathcch -lbcrypt -lm -Wl,--stack,2000000"

If I configure GDB without python support, then like gas and binutils the __imp_PathIsNetworkPathA symbol is not found.

In order to resolve the link issue, I think there are a couple of solutions.

1. Always link anything using libbfd against shlwapi - that feels a bit intrusive for one small change?
2. Implement an alternative to using PathIsNetworkPathA. From its description, I think it might be possible to substitute that call for something like `is_network_path = strncmp(filename, "//", 2) == 0 || strncmp(filename, "\\\\", 2) == 0` since I think that's the behaviour we actually care about.

Any thoughts?

(In reply to Simon Cook from comment #8)
 
> 2. Implement an alternative to using PathIsNetworkPathA. From its
> description, I think it might be possible to substitute that call for
> something like `is_network_path = strncmp(filename, "//", 2) == 0 ||
> strncmp(filename, "\\\\", 2) == 0` since I think that's the behaviour we
> actually care about.
> 
> Any thoughts?

Simpler is better in my opinion.  But since I am not a UNC expert I do not know if the above test is sufficient.  If it is then I would definitely suggest that we use it.

Hi Simon

 Attachment change was built success with (GDB14.2)
https://ftp.gnu.org/gnu/gdb/gdb-14.2.tar.gz 
 I didn't use python either. following are my build comands

  >> ./configure  --with-gmp-include=/home/zhiqxion/gmp --with-gmp-lib=/home/zhiqxion/gmp/.libs --with-mpfr-include=/home/zhiqxion/mpfr/src  --with-mpfr-lib=/home/zhiqxion/mpfr/src/.libs --libdir=/home/zhiqxion/libiconv/lib/.libs  --includedir=/home/zhiqxion/libiconv/lib/
  >> make -j 16

> 2. Implement an alternative to using PathIsNetworkPathA. From its
> description, I think it might be possible to substitute that call for
> something like `is_network_path = strncmp(filename, "//", 2) == 0 ||
> strncmp(filename, "\\\\", 2) == 0` since I think that's the behaviour we
> actually care about.
> 
> Any thoughts?

It is also possible by this way.

Created attachment 15460 [details]
Patch to fix issue without pulling in shlwapi

Patch with my suggested change, and done some testing pre-the previous fix and with my change and verified UNC paths still work. (Tested using ld loading static archives which was the case of this I found, I'm happy to do any before/after validation on GDB if someone has some steps to reproduce the original issue)

Submitting here since that's where the original patch was, but if you'd rather me send it to the list, I'm happy doing that also.

(In reply to Simon Cook from comment #12)
Hi Simon,

> Patch with my suggested change, and done some testing pre-the previous fix
> and with my change and verified UNC paths still work. (Tested using ld
> loading static archives which was the case of this I found, I'm happy to do
> any before/after validation on GDB if someone has some steps to reproduce
> the original issue)

+  bool is_network_path = strncmp (filename, "//", 2) == 0
+			 || strncmp (filename, "\\\\", 2) == 0;

Won't this test falsely match DOS paths that start with: \\?\, 
eg \\?\D:foo\bar  

IE shouldn't the test be:

  bool is_network_path = strncmp (filename, "//", 2) == 0
			 || strncmp (filename, "\\\\?\\UNC\\", 8) == 0;

Also, since we have a startswith() function the check could be simplified to:

 bool is_network_path = startswith (filename, "//")
		 || startswith (filename, "\\\\?\\UNC\\");


> Submitting here since that's where the original patch was, but if you'd
> rather me send it to the list, I'm happy doing that also.

Either place is good...

Cheers
  Nick

Created attachment 15464 [details]
V2 Patch to fix issue, and cover more path variants

I've thought about this more over the weekend, and refactored this a bit more to make it more robust, and more accurately cover relative paths when the current working directory isn't a DOS path. I've updated the comments to explain the methodology, but essentially these conversions are done based on the input string:

- \\?\ -> don't touch, already in form
- \\xyz\... -> \\?\UNC\xyz\... (keeping a single \ after UNC to keep it a valid input)
- C:\xyz... -> \\?\C:\...
- test.o, ../test.o, etc. -> check current working directory for correct prefix, such that if e.g. CWD is C:\ we end up with \\?\C:\test.o, or \\server\share\ becomes \\?\UNC\server\share\test.o

I've done some more testing with a small program feeding various strings into _bfd_real_fopen both when running from local disk and network share and I think the output is now correct in all cases. I did think could this be simplified as it feels like I'm repeating myself, but the side effect of the early tests being on char strings and the later tests being on wchar_t strings as provided by Windows is a bit unfortunate.

The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9dd918142787246ea7ed53494d9cbc6b51486133

commit 9dd918142787246ea7ed53494d9cbc6b51486133
Author: Nick Clifton <nickc@redhat.com>
Date:   Mon Apr 15 16:42:15 2024 +0100

    Remove dependency upon shlwapi library when building BFD for Windows/MinGW environments.
    
      PR 31527

Hi Simon,

  Thanks for the revised patch.  It looks a lot better to me now and so I have gone ahead and checked it in.

Cheers
  Nick

Hi.

The patch added the following:

```
  if (!file)
    perror("Error opening file");
```

This is problematic, as _bfd_real_fopen() is ultimately used for an example when ld is looking for an archive.  This means that perror() fires for soft failures, such as archive not found from a search directry entry (-L or --library-path).  

Again, on applications, that have multiple search path entries, in practice there will be always "Error opening file: No such file or directory" error messages displayed.  At least in this particular context those should be suppressed.

Created attachment 15466 [details]
Remove debug output

Oh drat, I missed that when I was scanning the final patch before submitting (scanned for explicit calls to printf, but missed I was also using perror), terribly sorry about that.

I've attached a patch to remove those two lines - would someone with commit access be able to apply that.

Oops - I missed that too.  Sorry.

Patch applied as commit: ab0a395b54d

Cheers
  Nick