[PATCH] D55510: [ExprConstant] Improve memchr/memcmp for type mismatch and multibyte element types

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.


================
Comment at: lib/AST/ExprConstant.cpp:6147-6148
+      return ZeroInitialization(E);
+    if (!Result.checkNullPointerForFoldAccess(Info, E, AK_Read))
+      return false;
+    QualType CharTy =
----------------
rsmith wrote:
> Why do we need to do this explicitly, rather than allowing to simply happen as part of the first `handleLValueToRValueConversion` below?
We want the error message to be produced even if we bail early on not having a valid designator.


================
Comment at: lib/AST/ExprConstant.cpp:6150
+    QualType CharTy =
+        Info.Ctx.getBaseElementType(getType(Result.getLValueBase()));
+    const bool IsRawByte = BuiltinOp == Builtin::BImemchr ||
----------------
rsmith wrote:
> 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.
Got it; I also updated the tests to be sensitive to this issue.


================
Comment at: test/CodeGenCXX/builtins.cpp:41
+    matchedFirstByteIn04030201();
+    // CHECK: call void @_ZN27MemchrMultibyteElementTests26matchedFirstByteIn04030201Ev()
+  }
----------------
rsmith wrote:
> hubert.reinterpretcast wrote:
> > rsmith wrote:
> > > 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.
> > 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.
> 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.
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));
}
```


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55510/new/

https://reviews.llvm.org/D55510