This is an archive of the discontinued LLVM Phabricator instance.

Make Type::GetByteSize optional
ClosedPublic

Authored by aprantl on Jan 25 2019, 5:31 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Jan 25 2019, 5:31 PM
labath accepted this revision.Jan 28 2019, 12:56 AM

Looks fine to me.

I guess this would be the place I'd start thinking about an OptionalSize class (to replace the two fields in the Type class). But I don't think that's necessary yet...

This revision is now accepted and ready to land.Jan 28 2019, 12:56 AM
clayborg added inline comments.
include/lldb/Symbol/Type.h
221 ↗(On Diff #183671)

Does this need to be uint64_t to share the same space as m_byte_size?

aprantl marked an inline comment as done.Jan 28 2019, 8:46 AM
aprantl added inline comments.
include/lldb/Symbol/Type.h
221 ↗(On Diff #183671)

I don't think so, but it might be more readable.

This revision was automatically updated to reflect the committed changes.

Pavel, I may need you help. I reverted the commit because it broke three PDB-related tests (SymbolFilePDBTests, func-symbols, and typedefs) that need more than just lld in order to run. The fix is probably trivial. We only need to set a breakpoint in Type.cpp:119 and every time a type with size 0 is passed in look at the frame we're coming from and make sure that that llvm::None is passed in instead of 0, similar to the changes in DWARFASTParserClang.cpp.

The attached patch is what it took to get the tests passing for me. It could use a bit of cleanup though.

Thanks a bunch, Pavel! That is very helpful.

I am not sure if you saw this, but it looks like it broke one of the tests on the Windows bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/1092