Is it time to allow StringRef to be constructed from nullptr?

While porting LLDB over to StringRef, I am continuously running into difficulties caused by the fact that StringRef cannot be constructed from nullptr. So I wanted to see peoples’ thoughts on removing this restriction from StringRef. To be clear, I’m only using LLDB as a motivating example, but I’m not requesting that it be done because LLDB is some kind of special case. If it is to be done it should be on its own merits. That said, here is some context:

LLDB has a lot of functions that look like this:

void foo(const char *, Bar, const char *).

I’m trying to port these to functions that look like this:

void foo(StringRef, Bar, StringRef).

Often times the parameters are string literals or char arrays, but equally often they are another const char* that got passed into the calling function, or a return value from a CRT function like strstr(), or many other possible sources. This latter category presents a problem for porting code to StringRef, because if I simply change the function signature and fix up compile errors, I will probably have introduced a bug because hundreds of callers will now be implicitly converting from const char* to StringRef, leaving open the possibility that one of those was null.

To work around this, I’ve started doing the following every time I port a function:

void foo(const char , Bar, const char) = delete;

This is pretty hackish, but it gets the job done. At least the compiler warns me and forces me to go inspect every callsite where there’s an implicit conversion. Unfortunately it also makes for extremely verbose code. Now instead of:

foo(“bar”, baz, “buzz”)

I have to write

foo(StringRef(“bar”), baz, StringRef(“buzz”))

even for string literals and char arrays, which will obviously never be null! If StringRef would handle a null argument gracefully, it would make my life much easier.

With that out of the way, here are some reasons I can see to allow StringRef accept null to its constructor which are independent of LLDB and stand on their own merit.

  1. std::string_view<> can be constructed with null. I don’t know when we will be able to use std::string_view<>, but there’s a chance that at some point in the future we may wish to remove StringRef in favor of string_view. That day isn’t soon, but in any case, it will be easier if our assumptions are the same.

  2. [nullptr, nullptr+0) is a valid range. Why shouldn’t we be able to construct a StringRef from an otherwise perfectly valid range?

  3. StringRef() can already be constructed from nullptr (!) Surprised? That’s what happens when you invoke the default constructor. It happily initializes the internal Data with null. So why not allow the same behavior when invoking the const char * constructor?

Thoughts?

Seems fine to me, I think the default constructor behavior makes it clear that this is fine. I don’t recall why the assert was originally added, but I don’t think it has added much (if any) value over the years.

-Chris

As a tangent: I don’t like the fact that StringRef is implicitly built out of “const char *”, this is calling strlen() and because it is implicit folks don’t realize when they go from string → char * → StringRef.
I rather have this constructor explicit, and provide an implicit one for string literal.

To come back to your point, I’m not sure if we should leave the internal pointer null or always set it to “”? This would provide the guarantee that dereferencing a StringRef is always valid without checking.

For now, leaving it returning null seems the least risk, since in theory someone could be relying on that (that would be horrible, of course, but I’ve seen worse).

I just noticed it’s not just the default constructor. Even the const char*, size_t constructor allows null as long as you specify a 0 length.

LLVM_ATTRIBUTE_ALWAYS_INLINE
/implicit/ StringRef(const char *data, size_t length)
: Data(data), Length(length) {
assert((data || length == 0) &&
“StringRef cannot be built from a NULL argument with non-null length”);
}

It seems that the only benefit of the asserting constructor is that it saves a branch when invoked with a character pointer (I would expect the branch to be optimized away when constructed with a string literal or char array).

While porting LLDB over to StringRef, I am continuously running into difficulties caused by the fact that StringRef cannot be constructed from nullptr. So I wanted to see peoples’ thoughts on removing this restriction from StringRef. To be clear, I’m only using LLDB as a motivating example, but I’m not requesting that it be done because LLDB is some kind of special case. If it is to be done it should be on its own merits. That said, here is some context:

LLDB has a lot of functions that look like this:

void foo(const char *, Bar, const char *).

I’m trying to port these to functions that look like this:

void foo(StringRef, Bar, StringRef).

Often times the parameters are string literals or char arrays, but equally often they are another const char* that got passed into the calling function, or a return value from a CRT function like strstr(), or many other possible sources. This latter category presents a problem for porting code to StringRef, because if I simply change the function signature and fix up compile errors, I will probably have introduced a bug because hundreds of callers will now be implicitly converting from const char* to StringRef, leaving open the possibility that one of those was null.

To work around this, I’ve started doing the following every time I port a function:

void foo(const char , Bar, const char) = delete;

This is pretty hackish, but it gets the job done. At least the compiler warns me and forces me to go inspect every callsite where there’s an implicit conversion. Unfortunately it also makes for extremely verbose code. Now instead of:

foo(“bar”, baz, “buzz”)

I have to write

foo(StringRef(“bar”), baz, StringRef(“buzz”))

even for string literals and char arrays, which will obviously never be null! If StringRef would handle a null argument gracefully, it would make my life much easier.

With that out of the way, here are some reasons I can see to allow StringRef accept null to its constructor which are independent of LLDB and stand on their own merit.

  1. std::string_view<> can be constructed with null. I don’t know when we will be able to use std::string_view<>, but there’s a chance that at some point in the future we may wish to remove StringRef in favor of string_view. That day isn’t soon, but in any case, it will be easier if our assumptions are the same.

  2. [nullptr, nullptr+0) is a valid range. Why shouldn’t we be able to construct a StringRef from an otherwise perfectly valid range?

  3. StringRef() can already be constructed from nullptr (!) Surprised? That’s what happens when you invoke the default constructor. It happily initializes the internal Data with null. So why not allow the same behavior when invoking the const char * constructor?

Thoughts?

As a tangent: I don’t like the fact that StringRef is implicitly built out of “const char *”, this is calling strlen() and because it is implicit folks don’t realize when they go from string → char * → StringRef.
I rather have this constructor explicit, and provide an implicit one for string literal.

I wonder if we could change that call site to be deleted (or at least explicit), and add support for literal strings with a StringRef version of this:

/// Construct an ArrayRef from a C array.
template <size_t N>
/implicit/ LLVM_CONSTEXPR ArrayRef(const T (&Arr)[N])
: Data(Arr), Length(N) {}

This way we’ll avoid the strlen on quoted strings which is the common case anyway, and then can see how many other cases we have from const char* remaining.

Pete

I thought about doing something like that, but most compilers will fold a call to strlen on a string literal into a constant anyway, so in practice I don’t think it matters much. I know Clang does, and I tested MSVC and it does too.

D:>type strlen.cpp
#include <string.h>
#include <stdio.h>

int main(int argc, char **argv) {
int x = strlen(“This is a test”);
printf(“%d”, x);
return 0;
}

D:>cl /O2 strlen.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24213.1 for x86
Copyright (C) Microsoft Corporation. All rights reserved.

strlen.cpp
Microsoft (R) Incremental Linker Version 14.00.24213.1
Copyright (C) Microsoft Corporation. All rights reserved.

/out:strlen.exe
strlen.obj

D:>dumpbin strlen.obj /disasm | grep -C 5 main
00000018: FF 30 push dword ptr [eax]

0000001A: E8 00 00 00 00 call ___stdio_common_vfprintf
0000001F: 83 C4 18 add esp,18h
00000022: C3 ret

_main:
00000000: 6A 0E push 0Eh
00000002: 68 00 00 00 00 push offset ??_C@_02DPKJAMEF@?$CFd?$AA@
00000007: E8 00 00 00 00 call _printf
0000000C: 83 C4 08 add esp,8
0000000F: 33 C0 xor eax,eax

Also, IANALL, but I don’t believe you can overload on const char* vs. const char (&T)[N]. If you have both overloads, a string literal and char array will still select the const char* overload, at least in the tests I attempted.

I thought about doing something like that, but most compilers will fold a call to strlen on a string literal into a constant anyway, so in practice I don’t think it matters much. I know Clang does, and I tested MSVC and it does too.

D:>type strlen.cpp
#include <string.h>
#include <stdio.h>

int main(int argc, char **argv) {
int x = strlen(“This is a test”);
printf(“%d”, x);
return 0;
}

D:>cl /O2 strlen.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24213.1 for x86
Copyright (C) Microsoft Corporation. All rights reserved.

strlen.cpp
Microsoft (R) Incremental Linker Version 14.00.24213.1
Copyright (C) Microsoft Corporation. All rights reserved.

/out:strlen.exe
strlen.obj

D:>dumpbin strlen.obj /disasm | grep -C 5 main
00000018: FF 30 push dword ptr [eax]

0000001A: E8 00 00 00 00 call ___stdio_common_vfprintf
0000001F: 83 C4 18 add esp,18h
00000022: C3 ret

_main:
00000000: 6A 0E push 0Eh
00000002: 68 00 00 00 00 push offset ??_C@_02DPKJAMEF@?$CFd?$AA@
00000007: E8 00 00 00 00 call _printf
0000000C: 83 C4 08 add esp,8
0000000F: 33 C0 xor eax,eax

Also, IANALL, but I don’t believe you can overload on const char* vs. const char (&T)[N]. If you have both overloads, a string literal and char array will still select the const char* overload, at least in the tests I attempted.

Except if the “const char *” one is made explicit :slight_smile:
I’m currently trying to make this change and it is… interesting!

To be clear: I’m not worried that this would add cost to the literal case. And the other change I’m suggesting is (relatively) orthogonal and isn’t a blocker for what you want to do.

To be clear: I’m not worried that this would add cost to the literal case.

Same here. Strlen should be optimised easily enough, and is from what you’ve found. If anything, all this will do is move that optimisation from the optimiser to the front end which is fine IMO. Might even speed up debug builds just a little :slight_smile:

And the other change I’m suggesting is (relatively) orthogonal and isn’t a blocker for what you want to do.

This is my view too. StringRef(nullptr) is reasonable even if we leave the literal behaviour the same for now.

Cheers
Pete

The pointer could only be null if the length is zero. If the length is zero, you shouldn’t be loading it. Defaulting to null instead of “” is also a microoptimization.

-Chris

The problem is that for a lot of functions, esp. the mem* family, NULL
as argument is triggering UB, even if size is 0. Since the GNU folks
started to attribute the functions like that, certain compilers will
pretty aggressively fold code based on that.

Joerg

I put up a CL to remove the assert

https://reviews.llvm.org/D24904

I’m not seeing much objection to this proposal but I posted on a weekend, so maybe someone will have something to say now. If there’s no objections I’ll wait a few hours after getting an lgtm to commit

The pointer could only be null if the length is zero. If the length is zero, you shouldn’t be loading it.

I was thinking about functions that takes a non-null pointer. For example, assuming you have:

extern void *my_memcpy (void *dest, const void *src, size_t len) attribute((nonnull (1, 2)));

You can just be calling it with any StringRef without a null pointer check.

That said it is just a thought I had, I don’t have any data or actual example to back this up.

Defaulting to null instead of “” is also a microoptimization.

You mean: because initializing to 0 is faster than a constant?

The pointer could only be null if the length is zero. If the length is zero, you shouldn’t be loading it.

I was thinking about functions that takes a non-null pointer. For example, assuming you have:

extern void *my_memcpy (void *dest, const void *src, size_t len) attribute((nonnull (1, 2)));

You can just be calling it with any StringRef without a null pointer check.

That said it is just a thought I had, I don’t have any data or actual example to back this up.

Yeah, I’m not sure where this comes up, and whether memcpy should take null when the length is zero is also a highly contentious thing that flares up in debates periodically.

Defaulting to null instead of “” is also a microoptimization.

You mean: because initializing to 0 is faster than a constant?

Yes, 0 is a “load immediate”, initializing to “” requires materializing the address of a global.

-Chris

One place that might be relevant is the existing special handling in StringRef around memcmp to avoid undefined behavior due to null pointers. It hints that we’ve already hit this issue once.