Re: Building today's snapshot of GDB with MinGW

[Eli Zaretskii <eliz@gnu.org>]

> Date: Mon, 29 Jun 2020 21:27:32 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> 4. Running "maint selftests" produces several warnings and failures:

I took a closer look at the selftest failures unrelated to
architectures.  There are a few:

>   Self test failed: self-test failed at unittests/format_pieces-selftests.c:37
>   Self test failed: self-test failed at utils.c:2971
>   Self test failed: Could not convert character to `UTF-8' character set
>   Self test failed: self-test failed at unittests/scoped_fd-selftests.c:80

The one in utils.c:2971 tests gdb_realpath.  It fails because
gdb_realpath implementation on MS-Windows returns a file name with
backslashes, whereas the test compares the basename of the value
starting with the last slash.  The patch to fix that is attached at
the end of this message.

The UTF-8 conversion problem comes from the Rust tests.  I don't
really understand why would this fail, since this GDB is linked with
libiconv.  Tom, can I get some guidance for how to dig deeper into
this?

The test in scoped_fd-selftests.c fails because it does something
MS-Windows doesn't support:

  ::scoped_fd sfd (gdb_mkostemp_cloexec (filename));
  SELF_CHECK (sfd.get () >= 0);

  unlink (filename);

  gdb_file_up file = sfd.to_file ("rw");
  SELF_CHECK (file != nullptr);

Gnulib's mkostemp opens the file with O_RDRW; according to MS
documentation of fdopen, here:

  https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fdopen-wfdopen?view=vs-2019

fdopen fails if the MODE argument doesn't match the original open mode
of the file descriptor, so we must use "r+", not "rw" (which isn't
supported according to the MS docs).  The second patch below fixes
that failure.

Btw, all the tests in scoped_fd-selftests.c call unlink to delete the
temporary file before closing its descriptor.  Is that necessary for
the tests to work correctly?  On Windows these unlink calls all fail,
and the resulting temporary files are left behind and clutter the
directory.  If possible, I'd like to move them after we close the
file.  Is it okay to submit a patch which will do that on all
platforms?

The format_pieces-selftests.c failure is due to the %lld, %llx, and
similar descriptors that test 'long long' formats.  The reason is
clearly seen in the debugger, for this test:

  static void
  test_format_specifier ()
  {
    /* The format string here ends with a % sequence, to ensure we don't
       see a trailing empty literal piece.  */
    check ("Hello\\t %d%llx%%d%d", /* ARI: %ll */
      {
	format_piece ("Hello\t ", literal_piece, 0),
	format_piece ("%d", int_arg, 0),
	format_piece ("%llx", long_long_arg, 0), /* ARI: %ll */
	format_piece ("%%d", literal_piece, 0),
	format_piece ("%d", int_arg, 0),
      });
  }

Here's the relevant part of the debug session:

  (top-gdb) p pieces.m_pieces
  $2 = {<std::_Vector_base<format_piece, std::allocator<format_piece> >> = {
      _M_impl = {<std::allocator<format_piece>> = {<__gnu_cxx::new_allocator<format_piece>> = {<No data fields>}, <No data fields>}, _M_start = 0x178c5f98,
	_M_finish = 0x178c5fd4,
	_M_end_of_storage = 0x178c5ff8}}, <No data fields>}
  (top-gdb) p pieces.m_pieces._M_impl._M_start
  $3 = (std::_Vector_base<format_piece, std::allocator<format_piece> >::pointer) 0x178c5f98
  (top-gdb) p *pieces.m_pieces._M_impl._M_start
  $4 = {string = 0x178c5ad0 "Hello\t ", argclass = literal_piece,
    n_int_args = 0}
  (top-gdb) p pieces.m_pieces._M_impl._M_start[1]
  $5 = {string = 0x178c5ad8 "%d", argclass = int_arg, n_int_args = 0}
  (top-gdb) p pieces.m_pieces._M_impl._M_start[2]
  $6 = {string = 0x178c5adc "%I64x", argclass = long_long_arg, n_int_args = 0}
  (top-gdb) p pieces.m_pieces._M_impl._M_start[3]
  $7 = {string = 0x178c5ae2 "%%d", argclass = literal_piece, n_int_args = 0}
  (top-gdb) p pieces.m_pieces._M_impl._M_start[4]
  $8 = {string = 0x178c5ae6 "%d", argclass = int_arg, n_int_args = 0}

As you see, we replace "%llx" with "%I64x", and the comparison then
fails.  Would it be okay to fix this by providing a different
expected_pieces when USE_PRINTF_I64 is non-zero?

Here are the two patches I propose; ok to commit (with the
appropriate log messages)?

--- gdb/utils.c~	2020-06-29 04:47:10.000000000 +0300
+++ gdb/utils.c	2020-07-02 16:11:45.476031500 +0300
@@ -2967,8 +2967,20 @@ gdb_realpath_check_trailer (const char *
   size_t len = strlen (result.get ());
   size_t trail_len = strlen (trailer);
 
+#ifndef _WIN32
   SELF_CHECK (len >= trail_len
 	      && strcmp (result.get () + len - trail_len, trailer) == 0);
+#else
+  const char *res_trail = result.get () + len - trail_len;
+  int slash = res_trail[0];
+
+  SELF_CHECK (len >= trail_len
+	      && (strcmp (result.get () + len - trail_len, trailer) == 0
+		  || (strcmp (result.get () + len - trail_len + 1,
+			      trailer + 1) == 0
+		      && IS_DIR_SEPARATOR (slash)
+		      && IS_DIR_SEPARATOR (trailer[0]))));
+#endif
 }
 
 static void
--- gdb/unittests/scoped_fd-selftests.c~	2020-06-29 04:47:10.000000000 +0300
+++ gdb/unittests/scoped_fd-selftests.c	2020-07-02 16:30:55.733738900 +0300
@@ -76,7 +76,7 @@ test_to_file ()
 
   unlink (filename);
   
-  gdb_file_up file = sfd.to_file ("rw");
+  gdb_file_up file = sfd.to_file ("r+");
   SELF_CHECK (file != nullptr);
   SELF_CHECK (sfd.get () == -1);
 }