Page MenuHomePhabricator

Fix bug in printing ValueObjects and in PE/COFF Plugin

Authored by zturner on Nov 7 2018, 4:50 PM.



The first bug is that the implementation of GetCompilerType for ValueObjectVariable only requests the forward type. This is insufficient for printing enum decls. I suspect this works with the DWARF plugin because the DWARF plugin probably completes enums immediately. The PDB plugin implements enums similarly to other tag records though, and only completes them lazily. ValueObjectVariable then requests the forward compiler type, and makes no attempt to complete it, resulting in insufficient information to print out the value of the enum.

Note that in general, you need a full compiler type to print out the variable anyway. If it's a class type you need it to print out its children, and this already happens when the ValueObject calls GetNumChildren. So this particular change isn't actually invoking any additional work or parsing that wasn't already going to happen anyway, it just allows the case where a symbol file plugin parses enums lazily.

The second bug fixed here is in ObjectFilePECOFF. Both MSVC and clang-cl emit their bss section as a magic section called .data with a raw size of 0 and a file offset of 0. These 3 properties together are sufficient to indicate "this is actually a BSS section", but we had no mechanism of reading from such sections. So we would just try the default read algorithm, which looks at the size, sees that it's too small (0 bytes), and gives up. So we need to add support for this in ObjectFilePECOFF.

This fixes all of the outstanding printing bugs in the native pdb reading tests.

Diff Detail


Event Timeline

zturner created this revision.Nov 7 2018, 4:50 PM
zturner added a reviewer: lemo.Nov 7 2018, 4:51 PM
zturner updated this revision to Diff 173095.Nov 7 2018, 5:11 PM

I added a test specifically for the bss bug. Even though one of my existing broken tests became fixed, someone could come down the line and adjust the test and set one of the variables to 1 which would cause the data to go into a regular section. To make sure this doesn't happen, I made a specific test just for this, and used llvm-readobj to also check that that the size of the section is in fact 0. So this test will always fail if the section stops being a compressed bss section for whatever reason.

lemo added a comment.Nov 7 2018, 5:17 PM

Shouldn't we also handle the general case where raw size < virtual size? (which would naturally subsume this fix)

1073 ↗(On Diff #173089)

shouldn't this be a hard error instead of returning 0?

1077 ↗(On Diff #173089)

do we really need to return the available "bytes" if the full read size can't be fullfiled? (ie. shouldn't that be a hard error instead?)

zturner updated this revision to Diff 173103.Nov 7 2018, 5:37 PM

I actually found an even better fix for this. LLDB sections already have the notion of "zero fill sections", we just need to mark the .data section as a zero fill section in this condition. No need to override ReadSectionData

clayborg requested changes to this revision.Nov 8 2018, 9:50 AM

Very close, just use the layout type as mentioned in the inline comment.

70 ↗(On Diff #173103)

Change this to GetLayoutCompilerType(). Why? If you have just a class Foo and your variable's type is "Foo *", then you don't need to complete it. The layout type gets you the type that is needed in order to display the current variable. For enums this will do the right thing. Otherwise if you have 1000 variables that are all pointers, you will be completing all of the class types for no reason.

This revision now requires changes to proceed.Nov 8 2018, 9:50 AM
zturner updated this revision to Diff 173194.Nov 8 2018, 10:34 AM

Use the layout type instead of the full type, along with a comment explaining why this is necessary.

clayborg accepted this revision.Nov 8 2018, 10:42 AM
This revision is now accepted and ready to land.Nov 8 2018, 10:42 AM
This revision was automatically updated to reflect the committed changes.