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

[Pedro Alves <palves at redhat dot com>]

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