Page MenuHomePhabricator

Search variables based on clang::DeclContext and clang::Decl tree
ClosedPublic

Authored by paulherman on Sep 4 2015, 7:24 PM.

Details

Summary

SymbolFileDWARF now creates VarDecl and BlockDecl and adds them to the Decl tree. Then, in ClangExpressionDeclMap it uses the Decl tree to search for a variable. This fixes lots of variable scoping problems.

Diff Detail

Event Timeline

paulherman updated this revision to Diff 34098.Sep 4 2015, 7:24 PM
paulherman retitled this revision from to Search variables based on clang::DeclContext and clang::Decl tree.
paulherman updated this object.
paulherman updated this revision to Diff 34238.Sep 8 2015, 12:24 PM

Search variables based on clang::DeclContext and clang::Decl tree

paulherman updated this revision to Diff 34273.Sep 8 2015, 3:38 PM

Search variables based on clang::DeclContext and clang::Decl tree

paulherman updated this revision to Diff 34287.Sep 8 2015, 5:48 PM

Search variables based on clang::DeclContext and clang::Decl tree

paulherman updated this object.Sep 8 2015, 5:51 PM
paulherman added a reviewer: clayborg.
paulherman added a subscriber: lldb-commits.
paulherman updated this revision to Diff 34289.Sep 8 2015, 5:59 PM

Search variables based on clang::DeclContext and clang::Decl tree

jingham added a subscriber: jingham.

Sean should take a look at this as well, since it directly affects how the expression parser lookup works.

paulherman updated this revision to Diff 34352.Sep 9 2015, 10:55 AM

Search variables based on clang::DeclContext and clang::Decl tree

Rebased the patch.

clayborg requested changes to this revision.Sep 10 2015, 3:51 PM
clayborg edited edge metadata.

So a few themes I would like to get fixed:

  • Don't parse everything in DWARF in case we ever need it (like parsing all namespaces and their contained variables, parse and cache all Block CompilerDeclContext within Block, etc), but do this lazily.
  • Add functionality to the abstract CompilerDeclContext to be able to find things within it. If we have namespace "a" with 1000000 variables inside, we should only parse the variables we need and we should ask the CompilerDeclContext to find things for us instead of parsing all 1000000 variables and then finding "b" in that list of 1000000 variables. We should ask the CompilerDeclContext:
CompilerDecl var_decl = decl_ctx.FindVariable("a");
CompilerDeclList decl_list = decl_ctx.FindDecls("b");
  • Parse things lazily as we need them and try not to store them in objects if we don't need to.
include/lldb/Symbol/Block.h
362–366 ↗(On Diff #34352)

Why did you add this function? See the function just above it? Why did you not use that one? This should be removed and it should be lazily determined by Block::GetDeclContext(). The current implementation should be all you need.

368–372 ↗(On Diff #34352)

Remove this. Not needed. We should calculate the decl context in Block::GetDeclContext() as needed. No need to store this in the block permanently.

492 ↗(On Diff #34352)

Remove this and let Block::GetDeclContext() create one for you each time. No need to permanently store this in a block instance.

include/lldb/Symbol/ClangASTContext.h
1088–1176

Should probably move these to the .cpp file.

include/lldb/Symbol/Variable.h
179–183

Is a variable a really a clang::DeclContext? This seems wrong. We might need a CompilerDecl class, but this seems wrong to put this in a CompilerDeclContext because a clang::VarDecl isn't a clang::DeclContext, it is a clang::Decl. Am I wrong?

185–189

Can we get rid of this and lazily init it in GetVarDecl() with code like:

CompilerDeclContext 
GetVarDecl ()
{
    if (!m_var_decl)
    {
         SymbolFile *symfile = ...;
         if (symfile)
             m_var_decl = symfile->GetDeclContext();
    }
    return m_var_decl;
}
source/Expression/ClangExpressionDeclMap.cpp
1359

Use llvm casting:

ClangASTContext *ast_context =  llvm::dyn_cast_or_null<ClangASTContext>(compiler_decl_context.GetTypeSystem());
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
1339–1349

Let the blocks parse these lazily as needed. We don't need to compute the CompilerDeclContext for any block unless we ask it to. The "Block::GetDeclContext()" call already exists and "does the right thing". No need for this.

3733–3735

We shouldn't do this all the time. This will cause performance issues. If something needs a namespace, it should call through the existing SymbolFile virtual function:

lldb_private::CompilerDeclContext
FindNamespace (const lldb_private::SymbolContext& sc,
               const lldb_private::ConstString &name,
               const lldb_private::CompilerDeclContext *parent_decl_ctx) override;

If you need to lookup namespace "b" that is in namespace "b" (a::b) you would call the above function with: "a" as the name first and a CompilerDeclContext for the translation unit (we might need to add a function to lldb_private::CompileUnit:

class CompileUnit
{
    CompilerDeclContext
    GetDeclContext ();
}

And it will lazily compute its compiler decl context and return it:

CompilerDeclContext cu_decl_ctx = sc.comp_unit->GetDeclContext();

CompilerDeclContext a_decl_ctx = symfile->FindNamespace(sc, ConstString("a"), &cu_decl_ctx);
if (a_decl_ctx)
{
    CompilerDeclContext b_decl_ctx = symfile->FindNamespace(sc, ConstString("b"), &a_decl_ctx);
}

We need to avoid adding code that parses all namespaces within a compile unit just because we might need the information later.

3740–3801

So no clang:: code should exist in SymbolFileDWARF. Any code that uses "clang::" (which is a recent change) needs to be moved into DWARFASTParserClang for clang specific things.

I like to see abilities added to CompilerDeclContext that can discover the variable declarations inside a specific CompilerDeclContext.

Lets say we have:

namespace a 
{
    int aa;
}

We should be able to get a CompilerDeclContext for namespace "a" and then ask the "a_decl_ctx" to find a variable named "aa". This way we avoid having to parse all namespaces and all variables within that namespace (which could be a lot of variables). This means we would need to ask a CompilerDeclContext something like:

CompilerDeclContext a_decl_ctx = ...;
if (a_decl_ctx)
{
    CompilerDecl var_decl = a_decl_ctx.FindVariable("aa");

Under the covers the CompilerDeclContext would need to call back into its symbol file (the TypeSystem inside the CompilerDeclContext has a link back to its SymbolFile).

4111–4126

I would like to avoid any of this kind of "parse a bunch of information 99% of the debugger will never use and store it permanently in case it ever gets used" kind of thing and discover variables through CompilerDeclContext as mentioned above.

4276–4277

I would like to avoid any of this kind of "parse a bunch of information 99% of the debugger will never use and store it permanently in case it ever gets used" kind of thing and discover variables through CompilerDeclContext as mentioned above.

This revision now requires changes to proceed.Sep 10 2015, 3:51 PM
paulherman added a comment.EditedSep 10 2015, 4:09 PM

I agree with most of your comments and will fix them.

One thing I noticed is that Block::GetDeclContext() does not do the right thing (see inline comment).

About creating a CompilerDecl class, I agree with you. I used CompilerDeclContext since it was just a void* and it was convenient. Do you think that the FindVariable method should be in ClangASTContext since there is the need to actually interpret the CompilerDeclContext according to clang rules?

Regarding ParseImportedNamespace, it also deals with "using NS::var" directives. Is there a method similar to FindNamespace that I can use?

In SymbolFileDWARF::GetDeclContextDIEContainingDIE, is there a reason why DW_TAG_subprogram, DW_TAG_lexical_block, etc are not considered DeclContexts?

include/lldb/Symbol/Block.h
362–366 ↗(On Diff #34352)

This function is needed as currently it returns the block of the function, not the block of the variable. See example below:

void function() 
{ // block 1 start
    ...
    { // block 2 start
        { // block 3 start
             // if we ask GetDeclContext() it returns the decl context of block 1 instead of block 2
        } // block 3 end
    } // block 2 end
} // block 1 end

I agree with most of your comments and will fix them.

One thing I noticed is that Block::GetDeclContext() does not do the right thing (see inline comment).

Yep, we only ever needed the function decl context before this. We can rename Block::GetDeclContext() to be Block::GetFunctionDeclContext(), and fix all places that use it to use the new name. Then we modify Block::GetDeclContext() to do what you want it to do.

Also note that you don't want any methods on lldb_private classes to be named "GetClangXXX()" since these objects are abstracted from specific type systems, everything has to be generic and abstract.

About creating a CompilerDecl class, I agree with you. I used CompilerDeclContext since it was just a void* and it was convenient. Do you think that the FindVariable method should be in ClangASTContext since there is the need to actually interpret the CompilerDeclContext according to clang rules?

There will need to be a ClangASTContext::DeclContextXXXX call made, but this will need to be a TypeSystem virtual function that is overridden by ClangASTContext since it inherits from lldb_private::TypeSystem, but we should have an abstract call on CompilerDeclContext that initiates the search (CompilerDeclContext::FindVariable(...)), which in turn calls the "TypeSystem::DeclContextFindVariable(void *)". This then lets you get back to the clang specific ways of finding things.

We might want to go a bit more generic and make a function like:

class CompilerDeclContext
{
    CompilerDeclList FindDecls(const char *name);
}

Since we actually want to start returning more than 1 thing for a search by name and there might be a variable named "a" and a function named "a" in that decl context? Or two overloads of a function "a" in the decl context. So the question is: do we try and search for CompilerDecl objects in a CompilerDeclContext so we can find all things whose name is "a", or do we stick with more targeted things like "CompilerDeclContext::FindVariable(...)". If we go the FindVariable route, we can probably skip making the new CompilerDecl and CompilerDeclList classes and just return lldb::VariableSP or use a lldb_private::VariableList class.

Regarding ParseImportedNamespace, it also deals with "using NS::var" directives. Is there a method similar to FindNamespace that I can use?

We will need to add this to the CompilerDeclContext, TypeSystem and to the SymbolVendor/SymbolFile interfaces, all in abstract ways.

In SymbolFileDWARF::GetDeclContextDIEContainingDIE, is there a reason why DW_TAG_subprogram, DW_TAG_lexical_block, etc are not considered DeclContexts?

They currently are expecting it to ignore these things. The question is if that is correct or not. We will need to look at who is asking. At the very least we can add a bool that says "include_functions_blocks" and then include them in the response if true if there are too many places that are using this incorrectly. There are only 3 places that call this function so it wouldn't be hard to fix them up to do the right thing.

The other thing Sean Callanan and Jim Ingham wanted to pass along is we want lookups to happen in via the clang::ExternalASTSource function callbacks like:

namespace clang {
  class ExternalASTSource : public RefCountedBase<ExternalASTSource> {
    virtual bool
    FindExternalVisibleDeclsByName(const DeclContext *DC, DeclarationName Name);
  }
}

I didn't look too closely at how your lookups were done, I mostly looked at the overall CompilerDeclContext and DWARF parsing aspect, but that is their preference. These lookups are always very targeted and say "inside the decl context 'DC', find "Name".

paulherman added a comment.EditedSep 10 2015, 5:23 PM

I believe that the approach of CompilerDeclContext::FindDecls could be better. But then this kind of forces CompilerDeclContext to inherit CompilerDecl (a function is both a DeclContext and a Decl) and I believe that creating a class for each possible entity is an overkill.

I started looking at SymbolFileDWARF. In order to not have "clang::___", I believe that ParseVariableDIE and ParseFunctionBlocks should be moved to DWARFASTParser.

I don't really understand what Sean and Jim mean. I believe that ClangASTSource implements the interface ExternalASTSource and ClangExpressionDeclMap (which does the actual search) inherits ClangASTSource, so it should be already alright...?

EDIT
I worked on fixing some of the things. However, I always encounter the need to store a map from CompilerDecl to VariableSP since given a CompilerDeclContext (using the solution above) we can get the CompilerDecl. Where do you think it would be best to store this map? I thought about adding it to SymbolFileDWARF and adding a method VariableSP SymbolFileDWARF::GetVariableFromDecl(CompilerDecl decl).

The other solution idea I have is to use StackFrame::GetInScopeVariableList to get the list of variables and search through that list to see if a variable has a decl in the list returned by CompilerDeclContext::FindDeclsByName. This might be better since there already is a call to StackFrame::GetInScopeVariableList so that the variable dies are parsed (is there a better way of doing this?).

paulherman updated this revision to Diff 34609.Sep 11 2015, 5:31 PM
paulherman edited edge metadata.

[WIP] Search variables based on clang::DeclContext and clang::Decl tree

This revision fixes some of the comments. There are some things I'm not sure about. The problem is that at some point there will be the need to link decls with the object they represent (Function, Variable, CompileUnit, etc). Is the approach of getting the VariableSP from the TypeSystem the right one? Also, should ParseVariableDIE be moved to DWARFASTParser in order to create the decl there or should there only be a method CreateVariableDecl(VariableSP var)?

paulherman updated this revision to Diff 34616.Sep 11 2015, 7:37 PM
paulherman edited edge metadata.

Search variables based on clang::DeclContext and clang::Decl tree

I believe I fixed most of the comments. I removed the handling of using declarations for now and made the test an expected failure because of this. I think it would be better if the parsing of DW_TAG_imported_declaration and DW_TAG_imported_module is handled in a different diff because and let this one as is.

Generally looks good to me with a few minor comments, but please wait for a review from Greg or Jim.

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
3201

(nit): Please swap the order of the condition as ExtractFormValueAtIndex will be slower then the comparision

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3892–3893

This check don't make sense here because dw_offset_t is 32bit and and UserIDMatches expects a 64bit value ant checks the symbol file id against the upper 32bit.

test/lang/cpp/nsimport/TestCppNsImport.py
19

Please add a bug number, or at least a short comment about why this test is XFAIL-ed

Search variables based on clang::DeclContext and clang::Decl tree

I think this fixes the comments. It also adds a cache for clang::decl so that there is no memory wasted.

I believe that the approach of CompilerDeclContext::FindDecls could be better. But then this kind of forces CompilerDeclContext to inherit CompilerDecl (a function is both a DeclContext and a Decl) and I believe that creating a class for each possible entity is an overkill.

I started looking at SymbolFileDWARF. In order to not have "clang::___", I believe that ParseVariableDIE and ParseFunctionBlocks should be moved to DWARFASTParser.

No they don't need to be. You just need to move the clang specific code into a function that is then passed through the TypeSystem interface. We don't need to make a CompilerDecl for a variable all the time, just when we ask the lldb_private::Variable class to get its CompilerDecl. So we could add method to Variable:

CompilerDecl
Variable::GetDecl();

It could always just call down to the symbol file and have the symbol file get the CompilerDecl for the variable. This would mean a new function on TypeSystem:

CompilerDecl
DeclGetDeclForVariable (const lldb::VariableSP &var_sp);

And that would eventually get routed down through the DWARFASTParser class...

I don't really understand what Sean and Jim mean. I believe that ClangASTSource implements the interface ExternalASTSource and ClangExpressionDeclMap (which does the actual search) inherits ClangASTSource, so it should be already alright...?

Yes the lookups currently happen like this and we should just be making function calls with any using directives to help fill in the results of this call.

EDIT
I worked on fixing some of the things. However, I always encounter the need to store a map from CompilerDecl to VariableSP since given a CompilerDeclContext (using the solution above) we can get the CompilerDecl. Where do you think it would be best to store this map? I thought about adding it to SymbolFileDWARF and adding a method VariableSP SymbolFileDWARF::GetVariableFromDecl(CompilerDecl decl).

If these are all abstract items, we can store any maps in TypeSystem itself.

The other solution idea I have is to use StackFrame::GetInScopeVariableList to get the list of variables and search through that list to see if a variable has a decl in the list returned by CompilerDeclContext::FindDeclsByName. This might be better since there already is a call to StackFrame::GetInScopeVariableList so that the variable dies are parsed (is there a better way of doing this?).

Yes, this would involve asking each VariableSP for its Decl using Variable::GetDecl() as mentioned above.

[WIP] Search variables based on clang::DeclContext and clang::Decl tree

This revision fixes some of the comments. There are some things I'm not sure about. The problem is that at some point there will be the need to link decls with the object they represent (Function, Variable, CompileUnit, etc). Is the approach of getting the VariableSP from the TypeSystem the right one?

It is fine because each TypeSystem does have a link to its SymbolFile, so yes this will work.

Also, should ParseVariableDIE be moved to DWARFASTParser in order to create the decl there or should there only be a method CreateVariableDecl(VariableSP var)?

No. ParseVariableDIE shouldn't be making the CompilerDecl at all. We should do this only when we call the "Variable::GetDecl()". It should then route this through the TypeSystem from the variable type and ask the type system for the CompilerDecl. This will get routed to the SymbolFile and then to that will get routed to the DWARFASTParser (DWARFASTParserClang for this case).

[WIP] Search variables based on clang::DeclContext and clang::Decl tree

This revision fixes some of the comments. There are some things I'm not sure about. The problem is that at some point there will be the need to link decls with the object they represent (Function, Variable, CompileUnit, etc). Is the approach of getting the VariableSP from the TypeSystem the right one?

It is fine because each TypeSystem does have a link to its SymbolFile, so yes this will work.

Also, should ParseVariableDIE be moved to DWARFASTParser in order to create the decl there or should there only be a method CreateVariableDecl(VariableSP var)?

No. ParseVariableDIE shouldn't be making the CompilerDecl at all. We should do this only when we call the "Variable::GetDecl()". It should then route this through the TypeSystem from the variable type and ask the type system for the CompilerDecl. This will get routed to the SymbolFile and then to that will get routed to the DWARFASTParser (DWARFASTParserClang for this case).

ParseVariableDIE does need at some point to get the CompilerDecl (but it gets it from Variable::GetDecl which calls DWARFASTParser) in order to link the Decl to the Variable. Is there a better solution to do this? Also, I think you're not looking at the latest diff as most of these have been addressed.

clayborg accepted this revision.Sep 14 2015, 11:52 AM
clayborg edited edge metadata.

I believe you have addressed all of my concerns. Read through my previous comments and make sure, and if so, this is good to go. Nice patch, thanks for working through the abtracting issues we ran into and for making a great patch.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3892–3893

Per tberghammer's comment we might need to add a way to get a DIERef from a DWARFFormValue...

This revision is now accepted and ready to land.Sep 14 2015, 11:52 AM

I think the only concern left might be the call to DeclLinkToObject in ParseVariableDIE. It doesn't have anything to do with clang but I feel like there should be a way to do this lazily.

Otherwise, I believe that the comments have been addressed.

Thank you!

clayborg requested changes to this revision.Sep 14 2015, 1:09 PM
clayborg edited edge metadata.

Yes, it would be nice to not have to get a variable's decl right up front. Maybe the Variable::GetDecl() could register this mapping if/when the variable decl is made the first time is is asked for?

This revision now requires changes to proceed.Sep 14 2015, 1:09 PM
paulherman added a comment.EditedSep 14 2015, 1:38 PM

Are you suggesting that the DeclLinkToObject is moved inside the GetDecl call and I only remove that? This is the only place where GetDecl is called, so it would be equivalent from a computational point of view. This is because ClangASTContext::DeclContextFindDeclByName assumes that the Decl tree is computed (hence a call to ParseVariablesForContext was placed).

Are you suggesting that the DeclLinkToObject is moved inside the GetDecl call and I only remove that?

Yes, actually you can probably move it into SymbolFileDWARF::GetDeclForUID(). there should be a cache in this function right? If the item isn't in the cache then we cache it and call DeclLinkToObject().

BTW: DeclLinkToObject() might be able to be moved into the base TypeSystem class?

This is the only place where GetDecl is called, so it would be equivalent from a computational point of view.

Yep, don't care how it happens, I just want it to be lazy and only done if someone asks for it.

If I move the call to DeclLinkDeclToObject inside any of the GetDeclXXX methods, then there is no way to access the VariableSP needed for the call and it is probably harder to reconstruct the parameters needed for ParseVariableDIE starting from just the DIE itself.

Also, the call is already lazy since ParseVariableDIE is lazy (i.e. whenever the VariableSP is needed, the Decl is needed). I don't think there is any way to separate the call to DeclLinkDeclToObject from ParseVariableDIE, only to make sure that DeclLinkDeclToObject doesn't do any work if it is called with a previously encountered variable.

paulherman added a comment.EditedSep 14 2015, 4:41 PM

I thought about the problem a bit more and I believe that having the call to DeclLinkDeclToObject inside ParseVariableDIE makes sense since this gets called only when the variable will actually be used in a search together with its context. Also, since ParseVariableDIE is lazy, this makes the linking to be lazy.

I don't really see how one could do this inside GetClangDeclForDIE or inside Variable::GetDecl as there is no access to the VariableSP in there.

What can be done is to add the link in ClangExpressionDeclMap after the call to GetInScopeVariableList by iterating through that list as this call is needed to actually parse the variables anyway.

So I don't like it in ParseVariableDIE() because it means we must create the decl right away for the variable when parsing it. ParseVariableDIE is used to parse all variables everywhere and we don't need the CompilerDecl in order to display the Variable, so we don't need to create it right away. We can always associate it in the Variable::GetDecl() call.

So I don't like it in ParseVariableDIE() because it means we must create the decl right away for the variable when parsing it. ParseVariableDIE is used to parse all variables everywhere and we don't need the CompilerDecl in order to display the Variable, so we don't need to create it right away. We can always associate it in the Variable::GetDecl() call.

Okay, I understand that. In order to accomplish this, Variable should probably inherit from std::enable_shared_from_this so that we only have one SP to the variable. Is this acceptable?

We only really need variable to inherit from std::shared_from_this if we have any classes or API that use a "Variable *" as ivars or parameters. If we do, feel free to make it inherit from shared_from_this, but I would like to avoid it if possible to save on space. Also if you convert to shared_from_this, all construction of Variables must be done into shared pointers directly as they are constructed or things will crash.

I just ran a grep through the source. It seems that everywhere it is created as a shared_ptr. In a previous attempt I tried storing it in the TypeSystem map from decl to object as Variable* instead of VariableSP and it seemed like at some point between launching a query and getting the value of a variable there were no references so the pointer got deleted. Hence, I guess it is kinda needed here.

paulherman updated this revision to Diff 34768.Sep 14 2015, 5:55 PM
paulherman edited edge metadata.

Search variables based on clang::DeclContext and clang::Decl tree

Moved the DeclLinkToObject inside Variable::GetDecl.

tberghammer added inline comments.Sep 15 2015, 3:11 AM
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3892–3893

Using form_value.Reference() as a uid and then passing it to UserIDMatches still have the same issue as before, because form_value.Reference() will return a 32bit DIE offset (the return type might be 64bit, but the data will be only 32)

DIERef has a costructor taking a DWARFFormValue, please use that one and the get the UID from the DIERef (it will store the CU from the form_value, and know how to calculate UID based on it)

paulherman edited edge metadata.

Search variables based on clang::DeclContext and clang::Decl tree

Search variables based on clang::DeclContext and clang::Decl tree

This adds handling of imported declarations, but there is no call to actually process them as I am not sure where to do this. I believe that the right place is in SymbolFileDWARF::ParseVariablesForContext.

clayborg accepted this revision.Sep 15 2015, 2:59 PM
clayborg edited edge metadata.

Looks good as long as my inlined comment doesn't point out an error, then this is good to go! Good stuff.

source/Symbol/Variable.cpp
250–258

As long as calling DeclLinkToObject(...) more than once with the same args is ok, then this will be fine.

This revision is now accepted and ready to land.Sep 15 2015, 2:59 PM
paulherman updated this revision to Diff 34850.Sep 15 2015, 4:43 PM
paulherman edited edge metadata.

Search variables based on clang::DeclContext and clang::Decl tree

Rebased the patch.

paulherman closed this revision.Sep 15 2015, 4:45 PM

I am seeing line 89 fail on MacOSX from your new TestCppNsImport.py:

test_result = frame.EvaluateExpression("imported")
self.assertTrue(test_result.IsValid() and test_result.GetValueAsSigned() == 99, "imported = 99")

Ours is picking up the global "imported" and it is getting 89. Can you send me the log file from the following commands?:

cd lldb/test/lang/cpp/nsimport
make

Now find the LLDB that you built and use it to debug:

../...../lldb a.out
(lldb) b /break 0/
(lldb) r
(lldb) log enable -f /tmp/expr-log.txt lldb expr
(lldb) p imported
(lldb) log disable lldb expr

Then send me the "expr-log.txt".

On MacOSX, we find the global "imported" in the translation unit:

ClangExpressionDeclMap::FindExternalVisibleDecls[9] for 'imported' in a 'TranslationUnit'

CEDM::FEVD[9] Searching the root namespace
CEDM::FEVD[9] Found variable imported, returned static int &imported (original int)

I am wondering if this works for you because your debug info isn't correctly describing the "imported" global variable. Maybe the one in the translation unit got stripped since it wasn't used? You might need to use it with something like:

::imported = 123;

If you can send me the expression log and possibly your ELF a.out file, I might be able to figure out what is going on.

Greg

The test was supposed to be marked as an XFAIL. I'm currently writing a fix for this that reports ambiguity in a context and deals with imported decls.

Ah, the test_with_dsym_and_run_command() wasn't marked as fail...

I fixed the test with:

% svn commit
Sending test/lang/cpp/nsimport/TestCppNsImport.py
Transmitting file data .
Committed revision 247764.

Feel free to correct it. I would also say that we might want to split up the test into multiple test functions so we can test individual things:

  • that using namespace works with no ambiguity
  • that you can see variables by specifying the global namespace and by specifying the namespace (like I just added with "::imported" and "Imported::imported")
  • anything else I missed..

Please also look at http://reviews.llvm.org/D12897

It fixes the imported decls and some bugs that I have missed (evaluating Fun::fun_var reports an error in TestCppNsImport). Unfortunately I don't think there is a way to report ambiguity without for "imported" without breaking "::imported" (details in the diff above).