Page MenuHomePhabricator

[ast] CreateParameterDeclaration should use an appropriate DeclContext.
ClosedPublic

Authored by zturner on Dec 11 2018, 12:55 PM.

Details

Summary

Previously CreateParameterDeclaration was always using the translation unit DeclContext. We would later go and add parameters to the FunctionDecl, but internally clang makes a copy when you do this, and we'd end up with ParmVarDecl's at the global scope as well as in the function scope.

This fixes the issue. It's hard to say whether this will introduce a behavioral change in name lookup, but I know there have been several hacks introduced in previous years to deal with collisions between various types of variables, so there's a chance that this patch could obviate one of those hacks.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

zturner created this revision.Dec 11 2018, 12:55 PM
clayborg accepted this revision.Dec 11 2018, 2:17 PM

Looks good to me. Jim should ok as well.

This revision is now accepted and ready to land.Dec 11 2018, 2:17 PM
This revision was automatically updated to reflect the committed changes.

This change looks good to me too.

I don't think this will fix the parameter/local variable lookup inversion that we've been hacking around. The problem comes because to make an JITted object we can easily call we make a wrapper function with a simple argument list - that makes running it easy but means it doesn't share the original function's context. Then we smuggle all the local Decls into the wrapper function using the FindExternalLexicalDecls from our ClangASTSource. Unfortunately, that lookup gets consulted after the namespace/class lookup is done. So names in the current class or namespace context shadow local variables/arguments.

This will clean up the AST Context's we make but sadly I don't think it will affect the expression parser name lookup issues. If you wanted to test that proposition, you can do:

settings set target.experimental.inject-local-vars false

then try some tests where a local variable or parameter name shadows an ivar or type name in the current context. That will find the wrong variable when this is false. I'm sure there are also tests for this, but I don't remember where they are off-hand.

friss added a subscriber: friss.Dec 13 2018, 1:49 PM

Zachary, how did you figure out this can be an issue? Does it fix something we should be testing?

Zachary, how did you figure out this can be an issue? Does it fix something we should be testing?

Recently I added a target modules dump ast command and I've been using it to write tests against (see lldb/lit/SymbolFile/NativePDB for an example of what some of these tests look like). While experimenting with this command, I noticed that I had some ParmVarDecls under the translation unit decl (e.g. global scope), which obviously is something that isn't possible. I only tested this with the PDB parser, so I can't confirm whether the same situation could occur int he DWARF parser, but since this is strictly building the AST *after* we've parsed the debug info, I definitely think it would be a problem for DWARF.

So the million dollar question is: What happens when you have function parameters in the AST at global scope? I think it falls into "undefined behavior" territory - i.e. there's no way to know what clang will do, since you're not supposed to have that situation.

As for as testing, I've found this command extremely useful for writing ast reconstruction tests -- Do a few things in the debugger, run the command, FileCheck the output. I think "did we construct a valid AST from the debug info?" is a largely unexplored testing surface in LLDB, so this definitely opens up a lot of possibilities for testing on this front.

Test passes now. Thanks!

Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 3:25 PM