Page MenuHomePhabricator

[NativePDB] Process virtual bases in the correct order
ClosedPublic

Authored by aleksandr.urakov on Jan 18 2019, 2:54 AM.

Details

Summary

This patch makes virtual bases to be added in the correct order to the bases list. It is important because VTableContext (MicrosoftVTableContext in our case) uses then the order of virtual bases in the list to restore the virtual table indexes. These indexes are used then to resolve the layout of the virtual bases.

We haven't enough information about offsets of virtual bases regarding to the object (moreover, in a common case we can't rely on such information, see the example here: https://reviews.llvm.org/D53506#1272306 ), but there should be enough information to restore the layout of the virtual bases from the indexes in runtime. After D53506 this information is used whenever possible, so there should be no problems with virtual bases' fields reading.

I have some problems with the test, I'll describe them exactly in the test's text below. Do you have them too or is this a specific problem with my setup?

Diff Detail

Repository
rLLDB LLDB

Event Timeline

aleksandr.urakov marked 2 inline comments as done.Jan 18 2019, 2:58 AM
aleksandr.urakov added inline comments.
lit/SymbolFile/NativePDB/tag-types.cpp
5

Clang gives me an error about char16_t, char32_t etc., but if I use MSVC here, then the test compiles ok.

238–247

For both enums I have a "no type was found matching" error (both for Clang and MSVC).

Ping! Can you take a look, please?

The above suggestion is admittedly minor, but since it's both a minor performance improvement and a minor readability/maintainability improvement, I think it's probably worth doing.

lit/SymbolFile/NativePDB/tag-types.cpp
5

Usually this means clang cannot find your CRT headers. Can you run with -verbose? That should give you some insight into the environment we are running clang with, and might be a clue why it can't find the headers.

source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
54

Then, change this function to not pass is_virtual, only pass Optional<vtable_idx>.

60–63

vtable_idx.hasValue();

68–71

m_bases.push_back(std::make_pair(vtable_idx.getValueOr(0), std::move(base_spec)));

215
using vt = std::pair<uint64_t, std::unique_ptr<clang::CXXBaseSpecifier>>>;
std::stable_sort(m_bases, [](const vt &base1, const vt &base2) {
  return base1.first < base2.first;
  });
source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
54

I actually think it would be slightly better to use a vector<pair<>> here. The reason is that a) it will probably be slightly more efficient, because # of vbases is usually quite small (often just 1) and the overhead of a map is a net performance loss for small numbers of items. b) that people are often confused when seeing map instead of DenseMap and it might cause someone in the future to think they can change this to a DenseMap with no behavioral difference, even though the reason for using a map here is that it must be sorted.

How about just having this:

std::vector<std::pair<uint64_t, std::unique_ptr<clang::CXXBaseSpecifier>>> m_bases;

Comments on how to change the implementation below.

Sorry, due to the way Phabricator re-orders feedback across files when sending the email notification, if you're reading my comments in email you have to read the previous email from the bottom up.

Thanks for the comments, I think that they make sense. I've updated the patch.

aleksandr.urakov marked an inline comment as done.Jan 29 2019, 5:15 AM
aleksandr.urakov added inline comments.
lit/SymbolFile/NativePDB/tag-types.cpp
5

Can you give me little more hints on this, please? I've run the verbose build and it have given me the following:

compiling tag-types.cpp -> tag-types.cpp.tmp.exe-tag-types.obj
  Command Line: C:\Work\llvm\build_x64\bin\clang-cl.exe -m64 /Od /GS- /GR- /Z7 -Xclang -fkeep-static-consts /c /FoC:\Work\llvm\build_x64\tools\lldb\lit\SymbolFile\NativePDB\Output\tag-types.cpp.tmp.exe-tag-types.obj C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp
  Env:
    TMP = C:\Users\ALEKSA~1.URA\AppData\Local\Temp
    SystemRoot = C:\WINDOWS
    SystemDrive = C:
    INCLUDE = C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.16.27023\ATLMFC\include
              C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.16.27023\include
              C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\ucrt
              C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\shared
              C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\um
              C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\winrt
              C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\cppwinrt
    TEMP = C:\Users\ALEKSA~1.URA\AppData\Local\Temp
  STDOUT:

  STDERR:
    C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp(16,3) :  error: unknown type name 'char16_t'
      char16_t            C16;
      ^
    C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp(17,3) :  error: unknown type name 'char32_t'
      char32_t            C32;
      ^
    C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp(41,3) :  error: unknown type name 'char16_t'
      char16_t            *PC16;
      ^
    C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp(42,3) :  error: unknown type name 'char32_t'
      char32_t            *PC32;
      ^
    C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp(64,9) :  error: unknown type name 'char16_t'
      const char16_t            *PC16;
            ^
    C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp(65,9) :  error: unknown type name 'char32_t'
      const char32_t            *PC32;
            ^
    6 errors generated.

There is the file named uchar.h in the directory C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\ucrt, and it contains the typedefs for char16_t and char32_t, but I can't figure out, how the compiler could see it.

An interesting thing is that the compilation with the same command in the same console completes successfully:

C:\Work\llvm\build_x64\bin\clang-cl.exe -m64 /Od /GS- /GR- /Z7 -Xclang -fkeep-static-consts /c /FoC:\Work\llvm\build_x64\tools\lldb\lit\SymbolFile\NativePDB\Output\tag-types.cpp.tmp.exe-tag-types.obj C:\Work\llvm\tools\lldb\lit\SymbolFile\NativePDB\tag-types.cpp

Ping! What do you think about it?

Ping! What do you think about it?

Generally the change looks good, but I'm not sure why the compiler can't properly compile the source file. I know it's something to do with the environment, but it's hard to say what.

One idea would be to add -### to the end of your command line when running the command manually, and then update the build.py script so that it also runs -### from the command line and prints the output. Then maybe compare the two command lines and see if there's a difference.

Another idea would be to hack up the code in the build.py script that modifies the child process's environment. Start with the child environment hardcoded to your terminal's environment, and then delete variables until the problem reproduces. That should tell you what environment variable is causing the problem.

Thanks for the reply! It seems that after your hint I've figured it out. It's the environment variable VCINSTALLDIR. I've added it to the defaultenv list of the _get_visual_studio_environment function and the test passes now. What do you think about this solution?

And do I understand right, it means, that clang-cl needs MSVC installation to work with these types?

Thanks for the help with the tests, it looks like they are ok now! So can we proceed with this patch?

This revision was not accepted when it landed; it landed in state Needs Review.Feb 12 2019, 12:16 AM
This revision was automatically updated to reflect the committed changes.