* [PATCH] gdbsupport: change path_join parameter to std::vector<const char *>
@ 2022-07-19 14:27 Simon Marchi
2022-07-19 15:10 ` Bruno Larsen
2022-09-21 15:31 ` Tom Tromey
0 siblings, 2 replies; 6+ messages in thread
From: Simon Marchi @ 2022-07-19 14:27 UTC (permalink / raw)
To: gdb-patches; +Cc: Simon Marchi
From: Simon Marchi <simon.marchi@efficios.com>
When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single
character name, we hit this assertion failure:
$ ./gdb -q --data-directory=data-directory -nx ./x
/usr/include/c++/12.1.0/string_view:239: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<char>; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed.
The backtrace:
#3 0x00007ffff6c0f002 in std::__glibcxx_assert_fail (file=<optimized out>, line=<optimized out>, function=<optimized out>, condition=<optimized out>) at /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:60
#4 0x000055555da8a864 in std::basic_string_view<char, std::char_traits<char> >::operator[] (this=0x7fffffffcc30, __pos=1) at /usr/include/c++/12.1.0/string_view:239
#5 0x00005555609dcb88 in path_join[abi:cxx11](gdb::array_view<std::basic_string_view<char, std::char_traits<char> > const>) (paths=...) at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:203
#6 0x000055555e0443f4 in path_join<char const*, char const*> () at /home/simark/src/binutils-gdb/gdb/../gdbsupport/pathstuff.h:84
#7 0x00005555609dc336 in gdb_realpath_keepfile[abi:cxx11](char const*) (filename=0x6060000a8d40 "/home/simark/build/binutils-gdb-one-target/gdb/./x") at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:122
#8 0x000055555ebd2794 in exec_file_attach (filename=0x7fffffffe0f9 "./x", from_tty=1) at /home/simark/src/binutils-gdb/gdb/exec.c:471
#9 0x000055555f2b3fb0 in catch_command_errors (command=0x55555ebd1ab6 <exec_file_attach(char const*, int)>, arg=0x7fffffffe0f9 "./x", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:513
#10 0x000055555f2b7e11 in captured_main_1 (context=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1209
#11 0x000055555f2b9144 in captured_main (data=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1319
#12 0x000055555f2b9226 in gdb_main (args=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1344
#13 0x000055555d938c5e in main (argc=5, argv=0x7fffffffdcf8) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
The problem is this line in path_join:
gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
... where `path` is "x". IS_ABSOLUTE_PATH eventually calls
HAS_DRIVE_SPEC_1:
#define HAS_DRIVE_SPEC_1(dos_based, f) \
((f)[0] && ((f)[1] == ':') && (dos_based))
This macro accesses indices 0 and 1 of the input string. However, `f`
is a string_view of length 1, so it's incorrect to try to access index
1. We know that the string_view's underlying object is a null-terminated
string, so in practice there's no harm. But as far as the string_view
is concerned, index 1 is considered out of bounds.
This patch makes the easy fix, that is to change the path_join parameter
from a vector of to a vector of `const char *`. Another solution would
be to introduce a non-standard gdb::cstring_view class, which would be a
view over a null-terminated string. With that class, it would be
correct to access index 1, it would yield the NUL character. If there
is interest in having this class (it has been mentioned a few times in
the past) I can do it and use it here.
This was found by running tests such as gdb.ada/arrayidx.exp, which
produce 1-char long filenames, so adding a new test is not necessary.
Change-Id: Ia41a16c7243614636b18754fd98a41860756f7af
---
gdbsupport/pathstuff.cc | 8 ++++----
gdbsupport/pathstuff.h | 8 ++++----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
index af10c6ebd2e8..390f10b1b5e4 100644
--- a/gdbsupport/pathstuff.cc
+++ b/gdbsupport/pathstuff.cc
@@ -191,21 +191,21 @@ child_path (const char *parent, const char *child)
/* See gdbsupport/pathstuff.h. */
std::string
-path_join (gdb::array_view<const gdb::string_view> paths)
+path_join (gdb::array_view<const char *> paths)
{
std::string ret;
for (int i = 0; i < paths.size (); ++i)
{
- const gdb::string_view path = paths[i];
+ const char *path = paths[i];
if (i > 0)
- gdb_assert (path.empty () || !IS_ABSOLUTE_PATH (path));
+ gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
if (!ret.empty () && !IS_DIR_SEPARATOR (ret.back ()))
ret += '/';
- ret.append (path.begin (), path.end ());
+ ret.append (path);
}
return ret;
diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
index d01db89e085f..aa7b0ffa2fbd 100644
--- a/gdbsupport/pathstuff.h
+++ b/gdbsupport/pathstuff.h
@@ -67,7 +67,7 @@ extern const char *child_path (const char *parent, const char *child);
The first element can be absolute or relative. All the others must be
relative. */
-extern std::string path_join (gdb::array_view<const gdb::string_view> paths);
+extern std::string path_join (gdb::array_view<const char *> paths);
/* Same as the above, but accept paths as distinct parameters. */
@@ -78,10 +78,10 @@ path_join (Args... paths)
/* It doesn't make sense to join less than two paths. */
gdb_static_assert (sizeof... (Args) >= 2);
- std::array<gdb::string_view, sizeof... (Args)> views
- { gdb::string_view (paths)... };
+ std::array<const char *, sizeof... (Args)> views
+ { paths... };
- return path_join (gdb::array_view<const gdb::string_view> (views));
+ return path_join (gdb::array_view<const char *> (views));
}
/* Return whether PATH contains a directory separator character. */
base-commit: 68cffbbd4406b4efe1aa6e18460b1d7ca02549f1
--
2.36.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbsupport: change path_join parameter to std::vector<const char *>
2022-07-19 14:27 [PATCH] gdbsupport: change path_join parameter to std::vector<const char *> Simon Marchi
@ 2022-07-19 15:10 ` Bruno Larsen
2022-07-19 18:55 ` Simon Marchi
2022-09-21 15:34 ` Simon Marchi
2022-09-21 15:31 ` Tom Tromey
1 sibling, 2 replies; 6+ messages in thread
From: Bruno Larsen @ 2022-07-19 15:10 UTC (permalink / raw)
To: Simon Marchi, gdb-patches; +Cc: Simon Marchi
On 7/19/22 11:27, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single
> character name, we hit this assertion failure:
>
> $ ./gdb -q --data-directory=data-directory -nx ./x
> /usr/include/c++/12.1.0/string_view:239: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<char>; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed.
>
> The backtrace:
>
> #3 0x00007ffff6c0f002 in std::__glibcxx_assert_fail (file=<optimized out>, line=<optimized out>, function=<optimized out>, condition=<optimized out>) at /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:60
> #4 0x000055555da8a864 in std::basic_string_view<char, std::char_traits<char> >::operator[] (this=0x7fffffffcc30, __pos=1) at /usr/include/c++/12.1.0/string_view:239
> #5 0x00005555609dcb88 in path_join[abi:cxx11](gdb::array_view<std::basic_string_view<char, std::char_traits<char> > const>) (paths=...) at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:203
> #6 0x000055555e0443f4 in path_join<char const*, char const*> () at /home/simark/src/binutils-gdb/gdb/../gdbsupport/pathstuff.h:84
> #7 0x00005555609dc336 in gdb_realpath_keepfile[abi:cxx11](char const*) (filename=0x6060000a8d40 "/home/simark/build/binutils-gdb-one-target/gdb/./x") at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:122
> #8 0x000055555ebd2794 in exec_file_attach (filename=0x7fffffffe0f9 "./x", from_tty=1) at /home/simark/src/binutils-gdb/gdb/exec.c:471
> #9 0x000055555f2b3fb0 in catch_command_errors (command=0x55555ebd1ab6 <exec_file_attach(char const*, int)>, arg=0x7fffffffe0f9 "./x", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:513
> #10 0x000055555f2b7e11 in captured_main_1 (context=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1209
> #11 0x000055555f2b9144 in captured_main (data=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1319
> #12 0x000055555f2b9226 in gdb_main (args=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1344
> #13 0x000055555d938c5e in main (argc=5, argv=0x7fffffffdcf8) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
>
> The problem is this line in path_join:
>
> gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>
> ... where `path` is "x". IS_ABSOLUTE_PATH eventually calls
> HAS_DRIVE_SPEC_1:
>
> #define HAS_DRIVE_SPEC_1(dos_based, f) \
> ((f)[0] && ((f)[1] == ':') && (dos_based))
>
> This macro accesses indices 0 and 1 of the input string. However, `f`
> is a string_view of length 1, so it's incorrect to try to access index
> 1. We know that the string_view's underlying object is a null-terminated
> string, so in practice there's no harm. But as far as the string_view
> is concerned, index 1 is considered out of bounds.
>
> This patch makes the easy fix, that is to change the path_join parameter
> from a vector of to a vector of `const char *`. Another solution would
> be to introduce a non-standard gdb::cstring_view class, which would be a
> view over a null-terminated string. With that class, it would be
> correct to access index 1, it would yield the NUL character. If there
> is interest in having this class (it has been mentioned a few times in
> the past) I can do it and use it here.
>
> This was found by running tests such as gdb.ada/arrayidx.exp, which
> produce 1-char long filenames, so adding a new test is not necessary.
>
> Change-Id: Ia41a16c7243614636b18754fd98a41860756f7af
> ---
> gdbsupport/pathstuff.cc | 8 ++++----
> gdbsupport/pathstuff.h | 8 ++++----
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
> index af10c6ebd2e8..390f10b1b5e4 100644
> --- a/gdbsupport/pathstuff.cc
> +++ b/gdbsupport/pathstuff.cc
> @@ -191,21 +191,21 @@ child_path (const char *parent, const char *child)
> /* See gdbsupport/pathstuff.h. */
>
> std::string
> -path_join (gdb::array_view<const gdb::string_view> paths)
> +path_join (gdb::array_view<const char *> paths)
> {
> std::string ret;
>
> for (int i = 0; i < paths.size (); ++i)
> {
> - const gdb::string_view path = paths[i];
> + const char *path = paths[i];
>
> if (i > 0)
> - gdb_assert (path.empty () || !IS_ABSOLUTE_PATH (path));
> + gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>
> if (!ret.empty () && !IS_DIR_SEPARATOR (ret.back ()))
> ret += '/';
>
> - ret.append (path.begin (), path.end ());
> + ret.append (path);
> }
>
> return ret;
> diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
> index d01db89e085f..aa7b0ffa2fbd 100644
> --- a/gdbsupport/pathstuff.h
> +++ b/gdbsupport/pathstuff.h
> @@ -67,7 +67,7 @@ extern const char *child_path (const char *parent, const char *child);
> The first element can be absolute or relative. All the others must be
> relative. */
>
> -extern std::string path_join (gdb::array_view<const gdb::string_view> paths);
> +extern std::string path_join (gdb::array_view<const char *> paths);
>
> /* Same as the above, but accept paths as distinct parameters. */
>
> @@ -78,10 +78,10 @@ path_join (Args... paths)
> /* It doesn't make sense to join less than two paths. */
> gdb_static_assert (sizeof... (Args) >= 2);
>
> - std::array<gdb::string_view, sizeof... (Args)> views
> - { gdb::string_view (paths)... };
> + std::array<const char *, sizeof... (Args)> views
> + { paths... };
Very minor nit, this array used to be called "views" because it held string_views, the name
should probably be changed, maybe path_array or something?
Cheers!
Bruno Larsen
>
> - return path_join (gdb::array_view<const gdb::string_view> (views));
> + return path_join (gdb::array_view<const char *> (views));
> }
>
> /* Return whether PATH contains a directory separator character. */
>
> base-commit: 68cffbbd4406b4efe1aa6e18460b1d7ca02549f1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbsupport: change path_join parameter to std::vector<const char *>
2022-07-19 15:10 ` Bruno Larsen
@ 2022-07-19 18:55 ` Simon Marchi
2022-09-21 15:34 ` Simon Marchi
1 sibling, 0 replies; 6+ messages in thread
From: Simon Marchi @ 2022-07-19 18:55 UTC (permalink / raw)
To: Bruno Larsen, Simon Marchi, gdb-patches
On 2022-07-19 11:10, Bruno Larsen wrote:
>
> On 7/19/22 11:27, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single
>> character name, we hit this assertion failure:
>>
>> $ ./gdb -q --data-directory=data-directory -nx ./x
>> /usr/include/c++/12.1.0/string_view:239: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<char>; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed.
>>
>> The backtrace:
>>
>> #3 0x00007ffff6c0f002 in std::__glibcxx_assert_fail (file=<optimized out>, line=<optimized out>, function=<optimized out>, condition=<optimized out>) at /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:60
>> #4 0x000055555da8a864 in std::basic_string_view<char, std::char_traits<char> >::operator[] (this=0x7fffffffcc30, __pos=1) at /usr/include/c++/12.1.0/string_view:239
>> #5 0x00005555609dcb88 in path_join[abi:cxx11](gdb::array_view<std::basic_string_view<char, std::char_traits<char> > const>) (paths=...) at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:203
>> #6 0x000055555e0443f4 in path_join<char const*, char const*> () at /home/simark/src/binutils-gdb/gdb/../gdbsupport/pathstuff.h:84
>> #7 0x00005555609dc336 in gdb_realpath_keepfile[abi:cxx11](char const*) (filename=0x6060000a8d40 "/home/simark/build/binutils-gdb-one-target/gdb/./x") at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:122
>> #8 0x000055555ebd2794 in exec_file_attach (filename=0x7fffffffe0f9 "./x", from_tty=1) at /home/simark/src/binutils-gdb/gdb/exec.c:471
>> #9 0x000055555f2b3fb0 in catch_command_errors (command=0x55555ebd1ab6 <exec_file_attach(char const*, int)>, arg=0x7fffffffe0f9 "./x", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:513
>> #10 0x000055555f2b7e11 in captured_main_1 (context=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1209
>> #11 0x000055555f2b9144 in captured_main (data=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1319
>> #12 0x000055555f2b9226 in gdb_main (args=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1344
>> #13 0x000055555d938c5e in main (argc=5, argv=0x7fffffffdcf8) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
>>
>> The problem is this line in path_join:
>>
>> gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>>
>> ... where `path` is "x". IS_ABSOLUTE_PATH eventually calls
>> HAS_DRIVE_SPEC_1:
>>
>> #define HAS_DRIVE_SPEC_1(dos_based, f) \
>> ((f)[0] && ((f)[1] == ':') && (dos_based))
>>
>> This macro accesses indices 0 and 1 of the input string. However, `f`
>> is a string_view of length 1, so it's incorrect to try to access index
>> 1. We know that the string_view's underlying object is a null-terminated
>> string, so in practice there's no harm. But as far as the string_view
>> is concerned, index 1 is considered out of bounds.
>>
>> This patch makes the easy fix, that is to change the path_join parameter
>> from a vector of to a vector of `const char *`. Another solution would
>> be to introduce a non-standard gdb::cstring_view class, which would be a
>> view over a null-terminated string. With that class, it would be
>> correct to access index 1, it would yield the NUL character. If there
>> is interest in having this class (it has been mentioned a few times in
>> the past) I can do it and use it here.
>>
>> This was found by running tests such as gdb.ada/arrayidx.exp, which
>> produce 1-char long filenames, so adding a new test is not necessary.
>>
>> Change-Id: Ia41a16c7243614636b18754fd98a41860756f7af
>> ---
>> gdbsupport/pathstuff.cc | 8 ++++----
>> gdbsupport/pathstuff.h | 8 ++++----
>> 2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
>> index af10c6ebd2e8..390f10b1b5e4 100644
>> --- a/gdbsupport/pathstuff.cc
>> +++ b/gdbsupport/pathstuff.cc
>> @@ -191,21 +191,21 @@ child_path (const char *parent, const char *child)
>> /* See gdbsupport/pathstuff.h. */
>> std::string
>> -path_join (gdb::array_view<const gdb::string_view> paths)
>> +path_join (gdb::array_view<const char *> paths)
>> {
>> std::string ret;
>> for (int i = 0; i < paths.size (); ++i)
>> {
>> - const gdb::string_view path = paths[i];
>> + const char *path = paths[i];
>> if (i > 0)
>> - gdb_assert (path.empty () || !IS_ABSOLUTE_PATH (path));
>> + gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>> if (!ret.empty () && !IS_DIR_SEPARATOR (ret.back ()))
>> ret += '/';
>> - ret.append (path.begin (), path.end ());
>> + ret.append (path);
>> }
>> return ret;
>> diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
>> index d01db89e085f..aa7b0ffa2fbd 100644
>> --- a/gdbsupport/pathstuff.h
>> +++ b/gdbsupport/pathstuff.h
>> @@ -67,7 +67,7 @@ extern const char *child_path (const char *parent, const char *child);
>> The first element can be absolute or relative. All the others must be
>> relative. */
>> -extern std::string path_join (gdb::array_view<const gdb::string_view> paths);
>> +extern std::string path_join (gdb::array_view<const char *> paths);
>> /* Same as the above, but accept paths as distinct parameters. */
>> @@ -78,10 +78,10 @@ path_join (Args... paths)
>> /* It doesn't make sense to join less than two paths. */
>> gdb_static_assert (sizeof... (Args) >= 2);
>> - std::array<gdb::string_view, sizeof... (Args)> views
>> - { gdb::string_view (paths)... };
>> + std::array<const char *, sizeof... (Args)> views
>> + { paths... };
>
> Very minor nit, this array used to be called "views" because it held string_views, the name
> should probably be changed, maybe path_array or something?
Good idea, I made the change locally.
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbsupport: change path_join parameter to std::vector<const char *>
2022-07-19 14:27 [PATCH] gdbsupport: change path_join parameter to std::vector<const char *> Simon Marchi
2022-07-19 15:10 ` Bruno Larsen
@ 2022-09-21 15:31 ` Tom Tromey
2022-09-21 15:38 ` Simon Marchi
1 sibling, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2022-09-21 15:31 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi, Simon Marchi
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
Simon> When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single
Simon> character name, we hit this assertion failure:
[...]
Simon> This patch makes the easy fix, that is to change the path_join parameter
Simon> from a vector of to a vector of `const char *`. Another solution would
Simon> be to introduce a non-standard gdb::cstring_view class, which would be a
Simon> view over a null-terminated string. With that class, it would be
Simon> correct to access index 1, it would yield the NUL character. If there
Simon> is interest in having this class (it has been mentioned a few times in
Simon> the past) I can do it and use it here.
I think it's fine to just do this for the time being.
Thank you.
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbsupport: change path_join parameter to std::vector<const char *>
2022-07-19 15:10 ` Bruno Larsen
2022-07-19 18:55 ` Simon Marchi
@ 2022-09-21 15:34 ` Simon Marchi
1 sibling, 0 replies; 6+ messages in thread
From: Simon Marchi @ 2022-09-21 15:34 UTC (permalink / raw)
To: Bruno Larsen, Simon Marchi, gdb-patches
On 2022-07-19 11:10, Bruno Larsen wrote:
>
> On 7/19/22 11:27, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single
>> character name, we hit this assertion failure:
>>
>> $ ./gdb -q --data-directory=data-directory -nx ./x
>> /usr/include/c++/12.1.0/string_view:239: constexpr const std::basic_string_view<_CharT, _Traits>::value_type& std::basic_string_view<_CharT, _Traits>::operator[](size_type) const [with _CharT = char; _Traits = std::char_traits<char>; const_reference = const char&; size_type = long unsigned int]: Assertion '__pos < this->_M_len' failed.
>>
>> The backtrace:
>>
>> #3 0x00007ffff6c0f002 in std::__glibcxx_assert_fail (file=<optimized out>, line=<optimized out>, function=<optimized out>, condition=<optimized out>) at /usr/src/debug/gcc/libstdc++-v3/src/c++11/debug.cc:60
>> #4 0x000055555da8a864 in std::basic_string_view<char, std::char_traits<char> >::operator[] (this=0x7fffffffcc30, __pos=1) at /usr/include/c++/12.1.0/string_view:239
>> #5 0x00005555609dcb88 in path_join[abi:cxx11](gdb::array_view<std::basic_string_view<char, std::char_traits<char> > const>) (paths=...) at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:203
>> #6 0x000055555e0443f4 in path_join<char const*, char const*> () at /home/simark/src/binutils-gdb/gdb/../gdbsupport/pathstuff.h:84
>> #7 0x00005555609dc336 in gdb_realpath_keepfile[abi:cxx11](char const*) (filename=0x6060000a8d40 "/home/simark/build/binutils-gdb-one-target/gdb/./x") at /home/simark/src/binutils-gdb/gdbsupport/pathstuff.cc:122
>> #8 0x000055555ebd2794 in exec_file_attach (filename=0x7fffffffe0f9 "./x", from_tty=1) at /home/simark/src/binutils-gdb/gdb/exec.c:471
>> #9 0x000055555f2b3fb0 in catch_command_errors (command=0x55555ebd1ab6 <exec_file_attach(char const*, int)>, arg=0x7fffffffe0f9 "./x", from_tty=1, do_bp_actions=false) at /home/simark/src/binutils-gdb/gdb/main.c:513
>> #10 0x000055555f2b7e11 in captured_main_1 (context=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1209
>> #11 0x000055555f2b9144 in captured_main (data=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1319
>> #12 0x000055555f2b9226 in gdb_main (args=0x7fffffffdb60) at /home/simark/src/binutils-gdb/gdb/main.c:1344
>> #13 0x000055555d938c5e in main (argc=5, argv=0x7fffffffdcf8) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
>>
>> The problem is this line in path_join:
>>
>> gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>>
>> ... where `path` is "x". IS_ABSOLUTE_PATH eventually calls
>> HAS_DRIVE_SPEC_1:
>>
>> #define HAS_DRIVE_SPEC_1(dos_based, f) \
>> ((f)[0] && ((f)[1] == ':') && (dos_based))
>>
>> This macro accesses indices 0 and 1 of the input string. However, `f`
>> is a string_view of length 1, so it's incorrect to try to access index
>> 1. We know that the string_view's underlying object is a null-terminated
>> string, so in practice there's no harm. But as far as the string_view
>> is concerned, index 1 is considered out of bounds.
>>
>> This patch makes the easy fix, that is to change the path_join parameter
>> from a vector of to a vector of `const char *`. Another solution would
>> be to introduce a non-standard gdb::cstring_view class, which would be a
>> view over a null-terminated string. With that class, it would be
>> correct to access index 1, it would yield the NUL character. If there
>> is interest in having this class (it has been mentioned a few times in
>> the past) I can do it and use it here.
>>
>> This was found by running tests such as gdb.ada/arrayidx.exp, which
>> produce 1-char long filenames, so adding a new test is not necessary.
>>
>> Change-Id: Ia41a16c7243614636b18754fd98a41860756f7af
>> ---
>> gdbsupport/pathstuff.cc | 8 ++++----
>> gdbsupport/pathstuff.h | 8 ++++----
>> 2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdbsupport/pathstuff.cc b/gdbsupport/pathstuff.cc
>> index af10c6ebd2e8..390f10b1b5e4 100644
>> --- a/gdbsupport/pathstuff.cc
>> +++ b/gdbsupport/pathstuff.cc
>> @@ -191,21 +191,21 @@ child_path (const char *parent, const char *child)
>> /* See gdbsupport/pathstuff.h. */
>>
>> std::string
>> -path_join (gdb::array_view<const gdb::string_view> paths)
>> +path_join (gdb::array_view<const char *> paths)
>> {
>> std::string ret;
>>
>> for (int i = 0; i < paths.size (); ++i)
>> {
>> - const gdb::string_view path = paths[i];
>> + const char *path = paths[i];
>>
>> if (i > 0)
>> - gdb_assert (path.empty () || !IS_ABSOLUTE_PATH (path));
>> + gdb_assert (strlen (path) == 0 || !IS_ABSOLUTE_PATH (path));
>>
>> if (!ret.empty () && !IS_DIR_SEPARATOR (ret.back ()))
>> ret += '/';
>>
>> - ret.append (path.begin (), path.end ());
>> + ret.append (path);
>> }
>>
>> return ret;
>> diff --git a/gdbsupport/pathstuff.h b/gdbsupport/pathstuff.h
>> index d01db89e085f..aa7b0ffa2fbd 100644
>> --- a/gdbsupport/pathstuff.h
>> +++ b/gdbsupport/pathstuff.h
>> @@ -67,7 +67,7 @@ extern const char *child_path (const char *parent, const char *child);
>> The first element can be absolute or relative. All the others must be
>> relative. */
>>
>> -extern std::string path_join (gdb::array_view<const gdb::string_view> paths);
>> +extern std::string path_join (gdb::array_view<const char *> paths);
>>
>> /* Same as the above, but accept paths as distinct parameters. */
>>
>> @@ -78,10 +78,10 @@ path_join (Args... paths)
>> /* It doesn't make sense to join less than two paths. */
>> gdb_static_assert (sizeof... (Args) >= 2);
>>
>> - std::array<gdb::string_view, sizeof... (Args)> views
>> - { gdb::string_view (paths)... };
>> + std::array<const char *, sizeof... (Args)> views
>> + { paths... };
>
> Very minor nit, this array used to be called "views" because it held string_views, the name
> should probably be changed, maybe path_array or something?
Done, thanks.
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdbsupport: change path_join parameter to std::vector<const char *>
2022-09-21 15:31 ` Tom Tromey
@ 2022-09-21 15:38 ` Simon Marchi
0 siblings, 0 replies; 6+ messages in thread
From: Simon Marchi @ 2022-09-21 15:38 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches
On 2022-09-21 11:31, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Simon> When a GDB built with -D_GLIBCXX_DEBUG=1 reads a binary with a single
> Simon> character name, we hit this assertion failure:
> [...]
>
> Simon> This patch makes the easy fix, that is to change the path_join parameter
> Simon> from a vector of to a vector of `const char *`. Another solution would
> Simon> be to introduce a non-standard gdb::cstring_view class, which would be a
> Simon> view over a null-terminated string. With that class, it would be
> Simon> correct to access index 1, it would yield the NUL character. If there
> Simon> is interest in having this class (it has been mentioned a few times in
> Simon> the past) I can do it and use it here.
>
> I think it's fine to just do this for the time being.
> Thank you.
I had forgotten about that patch, thanks. I also realized the subject is
wrong, it should be array_view instead of vector. Will push with that
fixed.
Simon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-21 15:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 14:27 [PATCH] gdbsupport: change path_join parameter to std::vector<const char *> Simon Marchi
2022-07-19 15:10 ` Bruno Larsen
2022-07-19 18:55 ` Simon Marchi
2022-09-21 15:34 ` Simon Marchi
2022-09-21 15:31 ` Tom Tromey
2022-09-21 15:38 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).