Page MenuHomePhabricator

[PDB] Make PDB lit tests use the new builder
ClosedPublic

Authored by aleksandr.urakov on Nov 27 2018, 3:18 AM.

Details

Summary

This patch makes old PDB plugin tests to use the new builder (see D54914).

There are some problems left:

  • Clang called from the builder shows errors on types like char16_t and char32_t. That's why enums-layout.test and typedefs.test are made to use MSVC instead;
  • The builder now doesn't support /Gy and /order options, so the old scheme is used in function-level-linking.test;
  • The builder now doesn't support compilation of multiple sources and linking them together, so the old scheme is used for linking in func-symbols.test;
  • On Windows 32-bit LLDB can debug only 32-bit applications, and 64-bit LLDB can debug only 64-bit applications. That's why the old scheme is left in dynamic tests udt-layout.test, variables-locations.test and vbases.test. If we will always compile the tests as 32-bit (64-bit), then they will fail on the 64-bit (32-bit) platform. That's why we just assume that the user runs the tests from the same command prompt where the build was run.

This one depends on D54914.

Diff Detail

Repository
rL LLVM

Event Timeline

This looks fine, but let's see if Zachary's change lands first and how it looks like after it lands :).

Clang called from the builder shows errors on types like char16_t and char32_t. That's why enums-layout.test and typedefs.test are made to use MSVC instead;

For me enums-layout.test crashes, but I see the same behavior in typedefs.test. This is because lld-link does not emit S_UDT records (note the original test was using %msvc_link, not %lld_link. That's why it passed before). I could probably add a new magic toolchain called clang-cl-with-msvc-linker but I think it's also fine to just keep these using cl.exe.

The builder now doesn't support /Gy and /order options, so the old scheme is used in function-level-linking.test;

Makes sense. Just curious, is the order file strictly necessary for this test? /Gy is the same as -ffunction-sections, so there could be an argument to be made for exposing --function-sections on the command line of build.py. On the other hand, there's no harm in falling back to the old mechanism here since this test isn't intended to be portable.

The builder now doesn't support compilation of multiple sources and linking them together, so the old scheme is used for linking in func-symbols.test;

This one is important to fix.

On Windows 32-bit LLDB can debug only 32-bit applications, and 64-bit LLDB can debug only 64-bit applications. That's why the old scheme is left in dynamic tests udt-layout.test, variables-locations.test and vbases.test. If we will always compile the tests as 32-bit (64-bit), then they will fail on the 64-bit (32-bit) platform. That's why we just assume that the user runs the tests from the same command prompt where the build was run.

Ahh, right. I wonder if we should have something like --arch=lldb that means "match the architecture of LLDB". It's not urgent for this patch, but it could be useful as a followup.

Thanks all!

Makes sense. Just curious, is the order file strictly necessary for this test? /Gy is the same as -ffunction-sections, so there could be an argument to be made for exposing --function-sections on the command line of build.py. On the other hand, there's no harm in falling back to the old mechanism here since this test isn't intended to be portable.

Yes, the order is necessary for the test, because it tests exactly correct processing of source files that are not continuous in the result binary. But I'm not against leaving it as is.

Ahh, right. I wonder if we should have something like --arch=lldb that means "match the architecture of LLDB". It's not urgent for this patch, but it could be useful as a followup.

Yes, I thought about a solution like these too. May be we could even make it the default arch?

Update tests due to D55230

It seems that all the reviews this one depends on are already in. Can we proceed with it?

This revision was not accepted when it landed; it landed in state Needs Review.Dec 5 2018, 9:38 AM
This revision was automatically updated to reflect the committed changes.

It looks strange. The compiler can't open the object file. And in the next build it is ok - may be it was some server failure (e.g. full disk)?

The similar problem with typedefs.test is here: http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1940/steps/test/logs/stdio

I have an assumption about the cause of the problem. Are the tests running in parallel? In this case typedefs.test and enums-layout.test are writing to the same object file together, because they both are compiled from the same source.

@zturner Is it possible to specify object file's name in compile-and-link mode? Then we can specify different names in different tests for both object files and executables. But I think that splitting the source or combining the tests would be a better idea.

I will fix it only tomorrow, because I'm already at home today. Feel free to revert it if needed.