[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

Artem Dergachev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 6 22:16:51 PDT 2021


NoQ added a comment.

Here's a reduced repro - a file that has different behavior before and after the patch (sorry, not perfectly reduced, my creduce is broken again):

  // RUN: clang --analyze %s
  typedef Oid;
  typedef Pointer;
  typedef text;
  errstart(int elevel, filename, lineno, funcname, domain);
  typedef Datum;
  typedef struct {
    Oid elemtype;
  } ArrayType;
  
  *guc_malloc(elevel, size) {
    void *data;
    data = malloc(size);
    if (data == (0))
      (errstart(elevel, "c", 3298, __func__, (0))
           ? ((errcode((((('0') & 0x3F) < 24)))))
           : 0);
    return (errstart(elevel, "c", 3324, __func__, (0)) ? ((errcode(((24))))) : 0);
  }
  
  ParseLongOption(*string, *name, value) {
    *name = guc_malloc(21, 1);
    __builtin___strlcpy_chk(*name, string, 1, string[1]);
  }
  
  ProcessGUCArray(ArrayType *array, action) {
    int i = 0;
    { (((((array)->elemtype) = 25)), "c", 7599); }
    for (i = i + 1; ((((char *)(array)) + sizeof(ArrayType)))[0];) {
      Datum d;
      char s;
      char name;
      char value;
      d = array_ref();
      s = text_to_cstring((text)((Pointer)(d)));
      ParseLongOption(s, &name, &value);
        (errstart(19, "c", 7629, __func__, (0)) ? ((errcode(), errmsg(name)))
                                                : 0);
    }
  }

It's most likely some budget heuristic acting up. By slightly changing stuff that has shouldn't have any effect you can easily eliminate the regression or even reverse it (so that the warning appeared before the patch but not after the patch). The patch definitely changes modeling so it's possible that it causes budgets to be spent differently.

One particular thing i noticed about the behavior after the patch (by observing exploded graph topologies) is that it causes states to be //merged// more often. Which is expected because where previously we've created a new extent symbol and added a  new constraint, currently we simply re-use the existing symbol. This causes 10% improvement in the number of generated nodes. I also didn't immediately notice any unintended state splits.

I think we should indeed move on. I'm curious which specific budget do we hit but this patch definitely didn't introduce the root cause of the problem, only accidentally surfaced it. We should still investigate the tracking bug though, i.e. why path in `guc_malloc()` isn't explained.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69726



More information about the cfe-commits mailing list