Skip to content

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Feb 13, 2020

Stumbled upon this while working on something else. This PR does three things:

  1. Move isAnonymous from UserType to Union.
  2. Fix the implementation of isAnonymous to match the behaviour of the extractor.
  3. Align the implementation of isAnonymous with the definition of anonymous unions in the C++14 standard.

The motivation for the first change is that the C++14 standard only mentions anonymity when talking about unions.

The motivation for the third change is that the following is not considered an anonymous union in the C++14 standard:

union {
  int i;
  float f;
} u;

@jbj
Copy link
Contributor

jbj commented Feb 13, 2020

What does this change mean for what I'd call an anonymous struct, then? As in

struct { int x; } s = { 5 };

And then there's https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html, which is apparently C11, and there's possibly also an MSVC language extension for it.

@MathiasVP
Copy link
Contributor Author

What does this change mean for what I'd call an anonymous struct, then? As in

struct { int x; } s = { 5 };

These are called unnamed classes in the standard [9.1.1]. I realize now that structs (obviously) also derived from UserType and so they lose the isAnonymous predicate given the current state of this PR.

And then there's https://gcc.gnu.org/onlinedocs/gcc/Unnamed-Fields.html, which is apparently C11, and there's possibly also an MSVC language extension for it.

That surprised me! I thought that clearly if the C++14 standard didn't include anonymous structs, then surely the C11 standard didn't either. But I was wrong!

struct {
  int a;
  struct {
    int a;
  };
} foo;

is (as far as I can read it) illegal C++14, but legal C11. To be faithful to the standards we could change it so that there's both an isAnonymous and an isUnnamed predicate on UserType.

But I also see that this might be an unnecessary breaking change. The least controversial (but still useful) change is probably to disregard the discrepancy between our predicate names and the standard, and just stick to fixing the string used for match.

(The reason I'm changing the definition of isAnonymous is that this has an affect on what is being considered to have internal linkage.)

@MathiasVP MathiasVP added the C++ label Feb 13, 2020
@jbj
Copy link
Contributor

jbj commented Feb 24, 2020

What's next for this PR?

@MathiasVP
Copy link
Contributor Author

What's next for this PR?

The main goal of this PR is really to define anonymous unions correctly such that it's possible to define a QL type for "variables with internal linkage".
This can then be used to remove the closed world assumption that's baked into
https://github.com/Semmle/ql/blob/2abe416670f68c25e01a69a173a57c9d97f465c4/cpp/ql/src/semmle/code/cpp/controlflow/internal/ConstantExprs.qll#L530

I think, however, that one simple thing to do is to not include anonymous unions in the definition of variables with internal linkage. In that case this PR isn't necessary for removing the closed world assumption, but the notion of internal linkage will be more restrictive than the standard defines it to be.

It might be useful to extract the bugfix to isAnonymous that I included in this PR, to a new and non-controversial PR.

@jbj
Copy link
Contributor

jbj commented Feb 24, 2020

Both options sound good to me.

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.

2 participants