Re: [PATCH 5/5] gdbsupport: add path_join function

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2022-04-15 01:59, Eli Zaretskii wrote:
>> Date: Thu, 14 Apr 2022 16:01:37 -0400
>> From: Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>>
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> In this review [1], Eli pointed out that we should be careful when
>> concatenating file names to avoid duplicated slashes.  On Windows, a
>> double slash at the beginning of a file path has a special meaning.  So
>> naively concatenating "/"  and "foo/bar" would give "//foo/bar", which
>> would not give the desired results.  We already have a few spots doing:
>>
>>   if (first_path ends with a slash)
>>     path = first_path + second_path
>>   else
>>     path = first_path + slash + second_path
>>
>> In general, I think it's nice to avoid superfluous slashes in file
>> paths, since they might end up visible to the user and look a bit
>> unprofessional.
>>
>> Introduce the path_join function that can be used to join multiple path
>> components together (along with unit tests).
> 
> Thanks.
> 
>> +static void
>> +test ()
>> +{
>> +  std::string s;
>> +
>> +  s = ::path_join ("/foo", "bar", nullptr);
>> +  SELF_CHECK (s == "/foo/bar");
>> +
>> +  s = ::path_join ("/foo", "/bar", nullptr);
>> +  SELF_CHECK (s == "/foo/bar");
>> +
>> +  s = ::path_join ("/", "bar", nullptr);
>> +  SELF_CHECK (s == "/bar");
>> +
>> +  s = ::path_join ("foo/", "/bar", nullptr);
>> +  SELF_CHECK (s == "foo/bar");
>> +
>> +  s = ::path_join ("foo", "bar", "", nullptr);
>> +  SELF_CHECK (s == "foo/bar");
>> +
>> +  s = ::path_join ("", "/foo", nullptr);
>> +  SELF_CHECK (s == "/foo");
>> +
>> +  s = ::path_join ("", "foo", nullptr);
>> +  SELF_CHECK (s == "foo");
>> +
>> +  s = ::path_join ("foo", "", "bar", nullptr);
>> +  SELF_CHECK (s == "foo/bar");
>> +
>> +  s = ::path_join ("foo", "", nullptr);
>> +  SELF_CHECK (s == "foo");
>> +
>> +  s = ::path_join ("foo/", "", nullptr);
>> +  SELF_CHECK (s == "foo/");
> 
> Suggest to add a couple of Windows-specific tests here: one which
> starts with "d:/" instead of just "/", and another with backslashes.

Should these tests only be ran on Windows hosts?  In an ideal world,
this function (and the rest of path handling in GDB) should support
cross-debugging both ways: GDB on Linux debugging a remote Windows
program, GDB on Windows debugging a remote Linux program.  That means
corner cases like GDB on Linux should not recognize C:/foo as an
absolute path when debugging a native Linux program, but it should
recognize it as an absolute path when debugging a remote Windows
program.  And it should recognize foo\bar\ as ending with a directory
separator when debugging remotely a Windows program, and not when
debugging natively.  And vice-versa for a Windows GDB / Linux program.

Existing code isn't written with that in mind though (macros like
IS_DIR_SEPARATOR are host-specific), and I'm not ready to tackle that
right now.   So I've written path_join ignoring those cross-debugging
scenarios.

So, if I add a test like:

  s = ::path_join ("foo\\", "bar", nullptr);
  SELF_CHECK (s == "foo\\bar" || s == "foo/bar");

it won't pass on Linux, as s will be "foo\\/bar".  So, as long as
path_join is host-dependent, I think we'll need to accept that some
tests will be host-dependent.  That's not ideal, but that's how things
are at the moment.

>> +std::string
>> +path_join (const char *component...)
>> +{
>> +  std::string path = component;
>> +
>> +  auto skip_leading_dir_seps = [] (const char *s)
>> +    {
>> +      while (*s == '/')
>> +	++s;
>> +
>> +      return s;
>> +    };
>> +
>> +  va_list args;
>> +  va_start (args, component);
>> +
>> +  const char *c = va_arg (args, const char *);
>> +  while (c != nullptr)
>> +    {
>> +      if (!path.empty ())
>> +	c = skip_leading_dir_seps (c);
>> +
>> +      if (*c != '\0')
>> +	{
>> +	  if (!path.empty () && path.back () != '/')
>> +	    path += '/';
>> +
>> +	  path += c;
>> +	}
>> +
>> +      c = va_arg (args, const char *);
>> +    }
>> +
>> +  va_end (args);
>> +
>> +  return path;
>> +}
> 
> I think this should use IS_DIR_SEPARATOR (or override the '=' and '!='
> operators), to support Windows file names with backslashes.

Changed both instances of '/' to use IS_DIR_SEPARATOR.

Simon