On May 20, 2011, at 11:49 AM, Matthieu Monrocq wrote: > 2011/5/20 Argyrios Kyrtzidis <[email protected]> > (cc'ing cfe-commits) > > On May 20, 2011, at 11:04 AM, Matthieu Monrocq wrote: > >> Hello, >> >> here is a second iteration of the patch >> >> 2011/5/19 Argyrios Kyrtzidis <[email protected]> >> (moved to cfe-commits) >> >>> /// Construct a string ref from a cstring. >>> /*implicit*/ StringRef(const char *Str) >>> - : Data(Str), Length(::strlen(Str)) {} >>> + : Data(Str), Length() { >>> + assert(Str && "StringRef cannot be built from a NULL argument"); >>> + Length = ::strlen(Str); // invoking strlen(NULL) is undefined >>> behavior >>> + } >>> >> >> >> "Length()" is not necessary. >> Could you also add an assert in the StringRef(const char *data, size_t >> length) constructor asserting that data is not null or length is 0 ? >> >> >> Removed and Done. >>> + >>> + // Workaround memcmp issue with null pointers (undefined behavior) >>> + // by providing a specialized version >>> + static int memcmp(const char *Lhs, const char *Rhs, size_t Length) { >>> + if (Length == 0) { return 0; } >>> + assert(Lhs && "memcmp - Lhs should be non-null when Length is not >>> 0"); >>> + assert(Rhs && "memcmp - Rhs should be non-null when Length is not >>> 0"); >>> + return ::memcmp(Lhs,Rhs,Length); >>> + } >>> + >> >> >> Is this really necessary ? With the 2 asserts in the constructors we are >> making sure that StringRefs point to non-null or their length is zero, and >> calling memcmp with zero length is defined, no ? >> >> >> I removed the two asserts since we now guarantee that Length is 0 if Data is >> null. >> >> I am afraid the check might be necessary, from n869 (a Draft of C99) >> >> > [7.21.1 String function conventions] >> > [#2] [...] Unless explicitly stated otherwise in the description of >> > a particular function in this subclause, pointer arguments on such a call >> > shall still have valid values, as described in 7.1.4. [...] >> >> [7.21.4.1 The memcmp function] does not state otherwise in any of its >> subclauses. >> >> I've hit the bug on Suse with the memcpy function (on a memcpy(NULL, NULL, >> 0) call) and am now paranoid about it. > > Ugh, that is good know. > But could you please rename 'memcmp' to something else (e.g. 'compareMemory') > ? I understand it was the choice with the least amount of changes, but it is > confusing, in general, to have member functions with the same name as > standard library functions. > > -Argyrios > > Done! > > There were only 4 call sites so not too invasive.
Committed in r131747, thanks! -Argyrios > > -- Matthieu > <llvm_stringref_undefined_behavior.diff>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
