This is an archive of the discontinued LLVM Phabricator instance.

[ExprConstant] Improve memchr/memcmp for type mismatch and multibyte element types
ClosedPublic

Authored by hubert.reinterpretcast on Dec 10 2018, 6:27 AM.

Details

Summary

memchr and memcmp operate upon the character units of the object representation; that is, the size_t parameter expresses the number of character units. The constant folding implementation is updated in this patch to account for multibyte element types in the arrays passed to memchr/memcmp and, in the case of memcmp, to account for the possibility that the arrays may have differing element types (even when they are byte-sized).

Actual inspection of the object representation is not implemented. Comparisons are done only between elements with the same object size; that is, memchr will fail when inspecting at least one character unit of a multibyte element. The integer types are assumed to have two's complement representation with 0 for false, 1 for true, and no padding bits.

memcmp on multibyte elements will only be able to fold in cases where enough elements are equal for the answer to be 0.

Various tests are added to guard against incorrect folding for cases that miscompile on some system or other prior to this patch. At the same time, the unsigned 32-bit wchar_t testing in test/SemaCXX/constexpr-string.cpp is restored.

Diff Detail

Event Timeline

aaron.ballman added inline comments.Dec 10 2018, 10:04 AM
lib/AST/ExprConstant.cpp
6151

Drop the top-level const.

8452

Drop top-level const. (Same suggestion anywhere the type is local to a function and is not a pointer or reference type.)

8465

Don't use auto here as the type is not spelled out in the initialization.

8469

Drop const and use a non-deduced type.

8470

Can you add a string literal to this assert explaining what's gone wrong if it triggers?

rsmith added inline comments.Dec 10 2018, 1:42 PM
lib/AST/ExprConstant.cpp
6147–6148

Why do we need to do this explicitly, rather than allowing to simply happen as part of the first handleLValueToRValueConversion below?

6150

Considering the LValue base here is incorrect. The object or array we're copying could be a subobject, and the complete object type (the type of the lvalue base) is irrelevant for such copies. Use Result.Designator.getType(Info.Ctx) to find the type that the lvalue designates. (You can bail out here if the designator is invalid; handleLValueToRValueConversion will always fail on such cases anyway.

6159–6160

Please add an assert here that the types do match in the non-raw-byte case. (They must, or the subobject designator would be invalid; the only special case is that a void* can point to anything, and only the memchr variants here take a void*.)

8437–8440

Per above comment, this is not correct.

8471

Style nit: please use while (true) rather than for (;;)

8475–8480

The *only* possible problem here is for bool, right? This comment would be clearer if it said that.

8488

Please add a comment before this return indicating that the result depends on byte order (and maybe a FIXME to compare the bytes in the right order rather than bailing out). We're going to need to deal with the byte order / representation problem for std::bit_cast pretty soon anyway.

8492–8493

This looks wrong. If 0 < MaxLength && MaxLength < LengthPerElement, we're supposed to compare *part of* the next element; this current approach doesn't do that. (If you did one more full-element compare, that'd be conservatively correct because we're only checking for equality, not ordering, but perhaps we should only permit cases where MaxLength is a multiple of LengthPerElement?

test/CodeGenCXX/builtins.cpp
41 ↗(On Diff #177498)

Please test this in a way that doesn't rely on IR generation constant-evaluating if conditions. Moreover, there's nothing about IR generation that you're actually testing here, so please phrase this as a Sema test instead (eg, check that a VLA bound gets folded to a constant). Likewise for the other tests below.

hubert.reinterpretcast marked 11 inline comments as done.

Make an initial pass at addressing the review comments

Address comments on style and code comments

lib/AST/ExprConstant.cpp
6159–6160

Will do.

8475–8480

We do assume two's complement representation (which C89 does not require to be true); so, bool is more problematic than other things, but the comment is not just about bool.

8492–8493

We are doing one more full-element compare to be conservatively correct. There is a test for that.

test/CodeGenCXX/builtins.cpp
41 ↗(On Diff #177498)

Not all of these cases can fold successfully with the current implementation. The check would need to be that we either fold (and get the right value), or don't fold at all.

rsmith added inline comments.Dec 10 2018, 3:39 PM
lib/AST/ExprConstant.cpp
8469

Use toCharUnitsFromBits here, and use CharUnits as a type-safe way of representing a size in terms of a number of chars. In fact, consider using getTypeSizeInChars a few lines above and only operating in chars, never bits.

8475–8480

There is no type other than bool that needs the extOrTrunc; maybe drop the "especially"?

8492–8493

Oh, I see now. Right. :)

Maybe add "Be careful not to compare more than MaxLength bytes" to the FIXME above, so a future maintainer doesn't get confused?

test/CodeGenCXX/builtins.cpp
41 ↗(On Diff #177498)

Seems fine to have a test that we don't fold certain cases at all, with a comment saying that it's OK if we start folding them to a constant and what that constant should be.

Generally the principle is that we want our unit tests to test as few layers of the stack as possible; the chosen test directory should ideally correspond to the layer under test. If you'd really like this to be more of an integration test, that's fine too (eg, if you have code like this in a platform header that really must be handled a certain way), but then it would belong in test/Integration. If you do put it in test/Integration, it'd then be reasonable to include -O in the test flags too, if that makes sense for what you want to test.

hubert.reinterpretcast marked 6 inline comments as done.

Use lvalue designator, add assertions for type matching

hubert.reinterpretcast marked an inline comment as done.Dec 10 2018, 4:39 PM
hubert.reinterpretcast added inline comments.
lib/AST/ExprConstant.cpp
6147–6148

We want the error message to be produced even if we bail early on not having a valid designator.

6150

Got it; I also updated the tests to be sensitive to this issue.

test/CodeGenCXX/builtins.cpp
41 ↗(On Diff #177498)

I think a SemaCXX test based on the following pattern would work:

constexpr decltype(sizeof 0) Good = 42, Bad = 43;
template <decltype(sizeof 0)> struct A;
struct NotBad {};
template <> struct A<Good> : NotBad {};

template <typename T, decltype(sizeof 0) N> A<N> *evalFor(T (&)[N]);
NotBad *evalFor(...);
void chk(NotBad *);

void f() {
  int x[__builtin_memcmp("", "", 1) == 0 ? Good : Bad];
  chk(evalFor(x));
}
hubert.reinterpretcast marked 3 inline comments as done.

Address remaining ExprConstant.cpp review comments

Use BytesRemaining/BytesPerElement to improve readability;
tweak FIXME comment to refer to the _remaining_ bytes.
Use CharUnits-to-CharUnits comparisons where straightforward.
Tweak comment about internal representation mismatch.

I'll update the tests to be in terms of constant/variable array length tomorrow. I think I've gotten through the rest of the comments.

Recast representation-sensitive tests as SemaCXX tests using array bounds

rsmith accepted this revision.Dec 11 2018, 4:27 PM
rsmith added inline comments.
lib/AST/ExprConstant.cpp
6147–6148

If we have an invalid designator, we already produced a diagnostic when setting it invalid, and generally don't need to diagnose anything else.

This revision is now accepted and ready to land.Dec 11 2018, 4:27 PM
hubert.reinterpretcast marked 3 inline comments as done.Dec 11 2018, 6:07 PM
hubert.reinterpretcast added inline comments.
lib/AST/ExprConstant.cpp
6147–6148

The null pointer diagnostic does not get produced if we do not do the check here on cases like the following:

static_assert(__builtin_memchr((int *)0, 0, 1) == 0, "");
hubert.reinterpretcast edited the summary of this revision. (Show Details)Dec 12 2018, 8:49 AM
This revision was automatically updated to reflect the committed changes.
hubert.reinterpretcast marked an inline comment as done.