PersistentSourceLocations are ambiguous

[rchyena]

Projects may use the C preprocessor to achieve polymorphic definitions. For example, consider this macro in safestack.h from OpenSSL:

# define SKM_DEFINE_STACK_OF(t1, t2, t3) \
    STACK_OF(t1); \
    typedef int (*sk_##t1##_compfunc)(const t3 * const *a, const t3 *const *b); \
    typedef void (*sk_##t1##_freefunc)(t3 *a); \
    typedef t3 * (*sk_##t1##_copyfunc)(const t3 *a); \
    static ossl_unused ossl_inline int sk_##t1##_num(const STACK_OF(t1) *sk) \
    { \
        return OPENSSL_sk_num((const OPENSSL_STACK *)sk); \
    } \
    <... snip ...>

This causes a problem for how we locate ConstraintVariables. We map PersistentSourceLocations to ConstraintVariables inside class ProgramInfo, but Clang places the spelling location of such definitions inside a scratch space, which can easily lead to namespace collisions. Testing against Icecast on Fedora 29, the following four (completely distinct) functions are associated with <scratch space>:12:1:

1. int i2d_PKCS7_SIGNED(PKCS7_SIGNED *a, unsigned char **out)
2. static inline int sk_OPENSSL_CSTRING_unshift(struct stack_st_OPENSSL_CSTRING *sk, const char *ptr) __attribute__((unused))
3. static inline struct stack_st_void *sk_void_new_null() __attribute__((unused))
4. void ASN1_IA5STRING_free(ASN1_IA5STRING *a)

Even if the compiler avoided the use of a scratch space for such functions, these collisions would continue to occur in the original file.

[Machiry]

It looks like the fix committed for this (ad94795) is causing regressions.
We see different AST ids for the same field of a structure :/

Example:

426278 {"line":"/home/machiry/checkedc/icecast_typedebugging/checkedc-icecast/src/common/httpp/httpp.h:83:12:37538","Variables":[{"PointerVar":{"Vars":["q_164350","q_164351"], "name":"value"}}]},
426279 {"line":"/home/machiry/checkedc/icecast_typedebugging/checkedc-icecast/src/common/httpp/httpp.h:83:12:66084","Variables":[{"PointerVar":{"Vars":["q_194595","q_194596"], "name":"value"}}]},
426280 {"line":"/home/machiry/checkedc/icecast_typedebugging/checkedc-icecast/src/common/httpp/httpp.h:83:12:86162","Variables":[{"PointerVar":{"Vars":["q_70368","q_70369"], "name":"value"}}]},
426281 {"line":"/home/machiry/checkedc/icecast_typedebugging/checkedc-icecast/src/common/httpp/httpp.h:83:12:86508","Variables":[{"PointerVar":{"Vars":["q_7399","q_7400"], "name":"value"}}]},
426282 {"line":"/home/machiry/checkedc/icecast_typedebugging/checkedc-icecast/src/common/httpp/httpp.h:83:12:90330","Variables":[{"PointerVar":{"Vars":["q_126661","q_126662"], "name":"value"}}]},
426283 {"line":"/home/machiry/checkedc/icecast_typedebugging/checkedc-icecast/src/common/httpp/httpp.h:83:12:93814","Variables":[{"PointerVar":{"Vars":["q_81379","q_81380"], "name":"value"}}]},

We hit this, while trying to convert the icecast source from: https://github.com/plum-umd/checkedc-icecast

Maybe we should use ids only for functions?

Attached is the zip file containing constraint_output.json generated while converting icecast code.

Ref: @hasantouma

icecast_constraint_output.json.zip

[john-h-kastner]

PR #266 improved how we deal with collisions between ambiguous source locations, but the root of the problem still exists. Source locations are still ambiguous. Ideally we can change the PersistentSourceLocation class in the future so that it uniquely identifies a point in the source.

[kyleheadley]

Are PersistantSourceLocations better understood now? If this is not a specific issue, we can close it or incorporate it into a larger plan to deal with other source location ambiguities.

[mwhicks1]

@jackastner I think this is now a non-issue? That is, the specific problem that came up with here is now fixed, even though rewriting has other problems? If so, please close. If not, please say what we still need to do.

[john-h-kastner]

The broader issue here is still relevant: a PersistentSourceLocationobject does not actually uniquely identify a location in the source code. We have, however, improved how we handle the ambiguity so that we get correct conversion.

I think this issue can closed, and it should be incorporated into an issue for emitting warnings when 3C can't rewrite a pointer that we think should be checked.