[PATCH] D75799: [JumpThreading] Don't create PHI nodes with "is.constant" values

[Bill Wendling via Phabricator via llvm-commits]

void added a comment.

In D75799#1913287 <https://reviews.llvm.org/D75799#1913287>, @joerg wrote:

> It still seems to be a perfectly reasonable optimisation under  the semantics of `is.constant`. The code here seems to be an abuse of it though and makes assumptions that are not sensible.


The code is abusi^Wusing how `__builtin_constant_p()` works in gcc. It's honored even after inlining. But while that's icky, what's going on here is not quite that.

          int val;
          if (get_user(len, optlen))
                  return -EFAULT;
  
          len = min_t(unsigned int, len, sizeof(int));
  
          if (len < 0)
                  return -EINVAL;
  
          switch (optname) {
               ...
          case TCP_ZEROCOPY_RECEIVE: {
                  struct tcp_zerocopy_receive zc;
                  int err;
  
                  if (get_user(len, optlen))
                          return -EFAULT;
                  if (len < offsetofend(struct tcp_zerocopy_receive, length))
                          return -EINVAL;
                  if (len > sizeof(zc)) {
                          len = sizeof(zc);
                          if (put_user(len, optlen))
                                  return -EFAULT;
                  }
                  if (copy_from_user(&zc, optval, len))
                          return -EFAULT;
                  lock_sock(sk);
                  err = tcp_zerocopy_receive(sk, &zc);
                  release_sock(sk);
                  if (len == sizeof(zc))
                          goto zerocopy_rcv_sk_err;
                  switch (len) {
                  case offsetofend(struct tcp_zerocopy_receive, err):
                          goto zerocopy_rcv_sk_err;
                  case offsetofend(struct tcp_zerocopy_receive, inq):
                          goto zerocopy_rcv_inq;
                  case offsetofend(struct tcp_zerocopy_receive, length):
                  default:
                          goto zerocopy_rcv_out;
                  }
  zerocopy_rcv_sk_err:
                  if (!err)
                          zc.err = sock_error(sk);
  zerocopy_rcv_inq:
                  zc.inq = tcp_inq_hint(sk);
  zerocopy_rcv_out:
                  if (!err && copy_to_user(optval, &zc, len))
                          err = -EFAULT;
                  return err;
          }

The `len` variable is assigned in several places here. In only one place does it get a constant value: the `len = sizeof(zc)` line. The `copy_from_user` and `copy_to_user` are inlined. They call a function (also inlined) to check the object size for overwrites:

  _Bool check_copy_size(const void *addr, unsigned long bytes, _Bool is_source)
  {
          int sz = __builtin_object_size(addr, 0);
          if (__builtin_expect(!!(sz >= 0 && sz < bytes), 0)) {
                  if (!__builtin_constant_p(bytes))
                          copy_overwrite(sz, bytes);
                  else if (is_source)
                          __bad_copy_from();
                  else
                          __bad_copy_to();
                  return false;
          }
          check_object_size(addr, bytes, is_source);
          return true;
  }

The `__builtin_constant_p(bytes)` needs to return `false`, otherwise we'll get a link error because the `__bad_copy_*` functions don't exist. (Note: `bytes` is `len` here.)

Now, because `len` doesn't change between the `copy_from_user` and `copy_to_user` calls, the check if it's constant is combined. Now jump threading comes along and essentially converts that code into one where the `if (len == sizeof(zc))` statement is split into two paths: one with `len` constant and the other with it non-constant. Those two combine into a PHI node (actually multiple PHI nodes) and that's what's used within the `copy_to_user` call. The `copy_from_user` call seems content to use the non-constant path (probably because of interaction between inlining and jump threading, because removing some of the calls between the two `copy_*_user` calls gets rid of the problem).

Threading through the `if (len == sizeof(zc))` statement would be fine if it weren't for the fact that programmers rely upon `is.constant` to be false when not all paths to it have constant values for the operand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75799