[lldb-dev] [BUG?] Confusion between translation units?

[Greg Clayton via lldb-dev]

> On Oct 21, 2015, at 8:45 AM, Ramkumar Ramachandra <artagnon at gmail.com> wrote:
> 
> So first, an addendum: I found a way to make the project build without
> using a symlink, and use a direct reference instead. The problem still
> persists. It may be that symlink is one of the problems, but it is
> certainly not the only problem.
> 
> On Tue, Oct 20, 2015 at 8:22 PM, Greg Clayton <gclayton at apple.com> wrote:
>> int
>> Declaration::Compare(const Declaration& a, const Declaration& b)
>> {
>>    int result = FileSpec::Compare(a.m_file, b.m_file, true);
>>    if (result)
> 
> Wait, won't FileSpec::Compare be true iff a.m_file is the same as
> b.m_file (excluding symlink resolution)?

No, it returns -1 for less than, 0 for equal and +1 for greater than.

> If so, why are we putting the symlink-checking logic in the true branch of the original
> FileSpec::Compare?

My concern is that it is expensive to be stat'ing files all the time and that it dirties the file cache on your system. 

Also many times the FileSpec objects are remote paths. What happens when you build your project and send someone the dSYM file? You sent me your dSYM file and I have no way to know if your two types were the same since they had different paths and I have no way to resolve your symbolic links. I would consider the types different.

We could modify FileSpec::Compare, but I would like to try to limit this to only happen for symbolic links if we do. We have also had problems with paths like:

/tmp/foo/../bar.txt 
/tmp/bar.txt

If they aren't resolved they won't compare correctly. Same with:

/tmp/./bar.txt 
/tmp/bar.txt

We don't want to go changing the FileSpec object on people by resolving the path all the time. Because sometimes people don't want the path changing since they might have other FileSpec objects that are encoded with "/tmp/./bar.txt" and they will expect a FileSpec object they create to maintain what they put into it. So if we can't update the FileSpec objects, then our compares would constantly have to try to "stat()" objects they may or may not have come from the current system. So we actually can't resolve them because of that. 

> Aren't we expanding the scope of what we match,
> instead of narrowing it?



> 
>>    {
>>        int symlink_result = result;
>>        if (a.m_file.GetFilename() == b.m_file.GetFilename())
>>        {
>>            // Check if the directories in a and b are symlinks to each other
>>            FileSpec resolved_a;
>>            FileSpec resolved_b;
>>            if (FileSystem::ResolveSymbolicLink(a.m_file, resolved_a).Success() &&
>>                FileSystem::ResolveSymbolicLink(b.m_file, resolved_b).Success())
>>            {
>>                symlink_result = FileSpec::Compare(resolved_a, resolved_b, true);
> 
> I'm confused. Shouldn't the logic be "check literal equality; if true,
> return immediately; if not, check equality with symlink resolution"?

These are compare routines that return -1, 0 or 1.

> 
>>            }
>>        }
>>        if (symlink_result != 0)
>>            return symlink_result;
>>    }
>>    if (a.m_line < b.m_line)
>>        return -1;
>>    else if (a.m_line > b.m_line)
>>        return 1;
>> #ifdef LLDB_ENABLE_DECLARATION_COLUMNS
>>    if (a.m_column < b.m_column)
>>        return -1;
>>    else if (a.m_column > b.m_column)
>>        return 1;
>> #endif
>>    return 0;
>> }
> 
> Here's my version of the patch, although I'm not sure when the code
> will be reached.
> 
> int
> Declaration::Compare(const Declaration& a, const Declaration& b)
> {
>    int result = FileSpec::Compare(a.m_file, b.m_file, true);
>    if (result)
>        return result;

The code in the if statement below is useless. If we reach this location, "result" is zero and the two file specs are equal.

>    if (a.m_file.GetFilename() == b.m_file.GetFilename()) {
>        // Check if one of the directories is a symlink to the other
>        int symlink_result = result;
>        FileSpec resolved_a;
>        FileSpec resolved_b;
>        if (FileSystem::ResolveSymbolicLink(a.m_file, resolved_a).Success() &&
>            FileSystem::ResolveSymbolicLink(b.m_file, resolved_b).Success())
>        {
>            symlink_result = FileSpec::Compare(resolved_a, resolved_b, true);
>            if (symlink_result)
>                return symlink_result;
>        }
>    }
>    if (a.m_line < b.m_line)
>        return -1;
>    else if (a.m_line > b.m_line)
>        return 1;
> #ifdef LLDB_ENABLE_DECLARATION_COLUMNS
>    if (a.m_column < b.m_column)
>        return -1;
>    else if (a.m_column > b.m_column)
>        return 1;
> #endif
>    return 0;
> }
> 
> If you're confident that this solves a problem, I can send it as a
> code review or something (and set up git-svn, sigh).

We actually can't really do this because we might have a dSYM file from another system that we are debugging locally so we can't actually rely on symlink resolving. We could try to ignore the path to the file and just make the decl file use only the basename. But this can cause problems when you actually have two copies of "Point.h" that have differing versions of "Point" like "struct Point { int x; int y; };" and "struct Point { int XXX; int YYY; };". The would both be defined in a file Point.h and lets say they are both defined on line 10, but they do differ. 

So the code you want to look into to see where the uniquing is in DWARFASTParserClang.cpp and the code is:

                    if (type_name_const_str &&
                        dwarf->GetUniqueDWARFASTTypeMap().Find (type_name_const_str,
                                                                die,
                                                                decl,
                                                                byte_size_valid ? byte_size : -1,
                                                                *unique_ast_entry_ap))
                    {
                        // We have already parsed this type or from another
                        // compile unit. GCC loves to use the "one definition
                        // rule" which can result in multiple definitions
                        // of the same class over and over in each compile
                        // unit.
                        type_sp = unique_ast_entry_ap->m_type_sp;
                        if (type_sp)
                        {
                            dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
                            return type_sp;
                        }
                    }

You can debug why this code fails. It is actually probably calling:


bool
lldb_private::operator == (const Declaration &lhs, const Declaration &rhs)
{
#ifdef LLDB_ENABLE_DECLARATION_COLUMNS
    if (lhs.GetColumn () == rhs.GetColumn ())
        if (lhs.GetLine () == rhs.GetLine ())
            return lhs.GetFile() == rhs.GetFile();
#else
    if (lhs.GetLine () == rhs.GetLine ())
        return FileSpec::Equal(lhs.GetFile(),rhs.GetFile(), true, true);
#endif
    return false;
}


So avoid any fixes to Declaration::Compare(). The FileSpec::Equal() will compare the following two correctly:

/tmp/foo/../bar.txt 
/tmp/bar.txt

But probably wouldn't catch:

/tmp/./bar.txt 
/tmp/bar.txt

Or your two paths... I would rather not start doing any extra file stat calls on systems since we can't trust the file paths originated on the current system, so I am not sure what the right solution is and would leave things as is.

It sounds like you fixed your symlink issue. So a few questions:
1 - do you have just one type now in your libmwcgir_vm_rt.dylib.dSYM when you type:

(lldb) image lookup -t "iplist<llvm::Function, llvm::ilist_traits<llvm::Function> >"

If so, then you will need to find other competing definitions in other shared libraries and see if any of them differ by comparing the full "clang_type" value.