This is an archive of the discontinued LLVM Phabricator instance.

Store decls in prototypes on the declarator instead of in the AST
ClosedPublic

Authored by rnk on Nov 30 2016, 1:23 PM.

Details

Summary

This saves two pointers (!) from FunctionDecl that were being used for
some rare and questionable C-only functionality. The
DeclsInPrototypeScope ArrayRef was added in r151712 in order to parse
this kind of C code:

enum e {x, y};
int f(enum {y, x} n) {
 return x; // should return 1, not 0
}

The challenge is that we parse 'int f(enum {y, x} n)' it its own
function prototype scope that gets popped before we build the
FunctionDecl for 'f'. The original change was doing two questionable
things:

  1. Saving all tag decls introduced in prototype scope on a TU-global

Sema variable. This is problematic when you have cases like this, where
'x' and 'y' shouldn't be visible in 'f':

void f(void (*fp)(enum { x, y } e)) { /* no x */ }

This patch fixes that, so now 'f' can't see 'x', which is consistent
with GCC.

  1. Storing the decls in FunctionDecl in ActOnFunctionDeclarator so that

they could be used in ActOnStartOfFunctionDef. This is just an
inefficient way to move information around. The AST lives forever, but
the list of non-parameter decls in prototype scope is short lived. By
moving this stuff to the Declarator, we get the right (short) lifetime.

As the original change was the author's first major Clang patch, they
can be forgiven.

Event Timeline

rnk updated this revision to Diff 79801.Nov 30 2016, 1:23 PM
rnk retitled this revision from to Store decls in prototypes on the declarator instead of in the AST.
rnk updated this object.
rnk added a reviewer: rsmith.
rnk added subscribers: cfe-commits, jmolloy.
thakis added a subscriber: thakis.Dec 2 2016, 12:48 PM

(Unrelated, but if you're looking at memory: When I was looking at it a while ago, IIRC a surprising amount of memory was taken up by CXXBasePaths objects, and just reordering fields to pack it better made that object several bytes smaller. IIRC I accidentally reverted my local patch for this, but if you're in the mood it's probably not too hard to recreate)

rnk updated this revision to Diff 80650.Dec 7 2016, 1:30 PM
  • Remove an assert and test that edge case
rsmith added inline comments.Dec 7 2016, 2:20 PM
include/clang/Sema/DeclSpec.h
1240

It seems plausible that generated code could have more than 256 such declarations. This class has a pointer, a set of bit-fields, 12 unsigneds, and another pointer, so we could give this a full 32 bits without increasing the size of the class. Alternatively we could share the storage with NumExceptions, since this can only be non-zero in C and that can only be non-zero in C++.

1311

Might make sense to put this into the preceding union, if we care about how big this type is.

test/Misc/ast-dump-decl.c
109–110

Why is this not here any more? Looks like SemaDecl.cpp:8255 should have added it to this DeclContext. It'd be nice for -ast-dump to still dump these declarations somewhere.

rnk marked an inline comment as done.Dec 7 2016, 3:57 PM
rnk added inline comments.
include/clang/Sema/DeclSpec.h
1240

I did it as a union with the EH machinery initially, but it changes our behavior on this test case:

void f(struct Foo {} o) {} // error in C++
struct Foo {}; // follow on error for redefining type Foo

We actually have test cases for this at clang/test/SemaCXX/type-definition-in-specifier.cpp. I don't think it's important to preserve that exact behavior, though, so performance may be more important. I just did it this way to avoid premature optimization.

test/Misc/ast-dump-decl.c
109–110

I changed things to dump non-parameter decls in the function now, but we only move NamedDecls into the function DeclContext, so the output is a little different. Think it matters?

rnk updated this revision to Diff 80682.Dec 7 2016, 3:57 PM
rnk marked an inline comment as done.
  • Allow more decls in prototypes, dump fd decls
rnk updated this revision to Diff 80827.Dec 8 2016, 2:26 PM
rnk marked an inline comment as done.
  • Reuse EH specifier storage in C
  • Revert dumper change
include/clang/Sema/DeclSpec.h
1240

As discussed offline, I updated the C++ test case and made this stuff not take up any space in C++ mode.

test/Misc/ast-dump-decl.c
109–110

I reverted my change to the dumper. Without more work, it's hard to know what decls came from the function prototype and which come from the function body. Filtering our parameters isn't enough. Think it's OK as is?

rsmith accepted this revision.Dec 8 2016, 7:37 PM
rsmith edited edge metadata.

I'd much prefer we take this size reduction now and worry about the minor loss of ast dump fidelity later. Please delete the commented out code though :)

This revision is now accepted and ready to land.Dec 8 2016, 7:37 PM
This revision was automatically updated to reflect the committed changes.