Re: [PATCH] Change some arguments to gdb::string_view instead of name+len

Pedro Alves
2019-10-01T19:10:07Z

On 10/1/19 7:27 PM, Christian Biesinger via gdb-patches wrote:
> On Tue, Oct 1, 2019 at 1:23 PM Pedro Alves <palves@redhat.com> wrote:
>>
>> On 10/1/19 6:33 PM, Christian Biesinger via gdb-patches wrote:
>>> - if (linkage_name[len] != '\0')
>>> + /* Don't use string_view::operator[] because we are accessing beyond
>>> + the size of the string_view, which is technically unsupported. */
>>> + if (linkage_name.data ()[linkage_name.length ()] != '\0')
>>> {
>>> char *alloc_name;
>>
>> It's more than just unsupported, it's undefined behavior. If we're promising
>> the string_view interface, then it's supposedly valid to pass in a string_view
>> that happens to point just at the end of a page, with the one-past-the-end
>> byte living in an unmapped page. Dereferencing the one-past-end byte in
>> that case SIGSEGVs.
>
> That's true (though also a pre-existing issue).
>
>>> - if (ms_type == mst_file_text && startswith (name, "__gnu_compiled"))
>>> + if (ms_type == mst_file_text && startswith (name.data (), "__gnu_compiled"))
>>> return (NULL);
>>>
>>
>> This, via startswith also assumes that name.data() is a null-terminated
>> string.
>
> Ah yes. I'll add a startswith version that takes string_views.
>
>> I wonder whether we should have a zstring_view type. like string_view, but
>> assumes/requires null-terminated.
>
> How does that solve anything? This function can (apparently) take
> non-null terminated strings, so zstring_view wouldn't work?

Ah, right.

Hmm.

I wonder then, I assume that the caller up the stack should know whether
the string was originally null terminated? I wonder about tweaking the
interface to pass that info down somehow.

Are those cases the ones where you call strlen at the caller?

Like, the interface could be:

/* ... If LEN is -1, then LINKAGE_NAME is a null-terminated string.
Otherwise, LINKAGE_NAME is a pointer to a string of LEN length,
and not null-terminated. ... */

void
symbol_set_names (struct general_symbol_info *gsymbol,
const char *linkage_name, int len, int copy_name,
struct objfile_per_bfd_storage *per_bfd)
{

So effectively, you'd be pushing the strlen call down to
symbol_set_names.

This goes against the idea of using string_view here, though...

Thanks,
Pedro Alves