Skip to content

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Mar 3, 2020

We've worked around several issues with malformed IR due to malformed databases (#2821, #2844, #2835), and some of these workarounds should be replaced with fixes at a deeper level.

This PR is an attempt to understand why variables (and therefore expressions) can get multiple types. Hopefully that will get us closer to proposing and implementing a fix, either at the extractor level or the QL level.

@jbj jbj added the C++ label Mar 3, 2020
@jbj jbj requested review from nickrolfe and dbartol March 3, 2020 13:51
@jbj jbj requested a review from a team as a code owner March 3, 2020 13:51
// BUG: types of members depend on extraction order.
// Only one copy of this struct is extracted, and the types of its members refer
// to the typedefs in file1.c. Had file2.c been extracted first instead, the
// types of its members would be different.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds right, and is similar to the way that the existing variables/global test would produce different output if a.c and b.c got extracted in a different order.

https://github.com/Semmle/ql/blob/88c74b2a4bb542f4c930bc5643eac3b11a358692/cpp/ql/test/library-tests/variables/global/variables.expected#L1-L2

// The type of `structMember` is ambiguous, and the two possible types are not
// unifiable, meaning in this context that they don't share an unspecified type.
// The types are nevertheless _compatible_, so it's valid C (not C++) to use
// these two definitions interchangably in the same program.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The content hashes for NotUnifiableTwice include the name emptyStruct1 or emptyStruct2, which is how they end up differing.

Copy link
Contributor

@nickrolfe nickrolfe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments you've added look right to me, and correspond with my understanding of the extractor and QL library's behaviour.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like it "Looks Good To Nick"

@MathiasVP MathiasVP merged commit f4e8f7a into github:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants