Page MenuHomePhabricator

Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC)
ClosedPublic

Authored by aprantl on Jan 14 2019, 4:21 PM.

Details

Summary

My apologies for the horrible patch.

The code in LLDB assumes that CompilerType and friends use the size 0 as a sentinel value to signal an error. This works for C++, where no zero-sized type exists, but in many other programming languages (including I believe C) types of size zero are possible and even common. This is a particular pain point in swift-lldb, where extra code exists to double-check that a type is *really* of size zero and not an error at various locations.

To remedy this situation, this patch starts by converting CompilerType::getBitSize() and getByteSize() to return an optional result. To avoid wasting space, I hand-rolled my own optional data type assuming that no type is larger than what fits into 63 bits. Follow-up patches would make similar changes to the ValueObject hierarchy.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Jan 14 2019, 4:21 PM

Overall, I am pretty sure we will end up with some kind of a special Optional class sooner or later. However, it's not clear to me whether this is the place to start. What kind of space this is saving? I would expect that size matters when you store something as a data member in a class, because you can have zillion of instances of that class on the heap. However, all the uses of this I see are local (stack) variables, where the size does not matter that much, and so you might as well have used an Optional. Have I missed something?

Overall, I think a good way to keep things sane would be to keep these specialized Optional classes as implementation details of classes, and convert to a real Optional as soon as it goes out to the rest of the world. This would be more consistent with how the rest of llvm does this (see llvm::VersionTuple for instance, which also does some storage optimizations, though without a special class). This would allow us to use the richer Optional interface in most of the places (e.g. Optional has special overloads of operator==, which allow you to compare an Optional<T> with plain T), while still keeping the size footprint in check.

+1. Especially since our assumption that everything related to C++ is of non-zero size is also not entirely correct. Empty structs for example are actually size 0 for Clang until we reach code gen where we set their size to 1. That's why we have this bug where we can't call functions that return an empty struct.

aprantl updated this revision to Diff 181797.Jan 15 2019, 8:49 AM

Thanks Pavel, your argument of optimizing storage over interfaces makes perfect sense. Using an Optional instead.

davide accepted this revision.Jan 15 2019, 9:36 AM

I hit an incarnation of this problem, and I was able to get away with a localized fix in:
[lldb] r344982 - [ValueObject] Stop assuming types are non-zero sized.

Of course, your approach is principled and it's IMHO clearly the right way forward.
I didn't check every single callsite, as the patch is mechanical enough, but the general idea looks good to me.

This revision is now accepted and ready to land.Jan 15 2019, 9:36 AM
clayborg accepted this revision.Jan 15 2019, 9:47 AM

Looks good to me

labath accepted this revision.Jan 15 2019, 9:59 AM
This revision was automatically updated to reflect the committed changes.

I had to stop commenting because there's too many occurrences, but please remove auto pretty much throughout the patch. A reader of the code will not necessarily be aware or remember that the function returns an Optional, and they will read the code as "if the size is 0, report an error". So I think the type should be spelled explicitly in all cases.

include/lldb/Target/ProcessStructReader.h
70 ↗(On Diff #181797)

I think we shouldn't use auto here, although this is a minor nit and I don't feel strongly.

73 ↗(On Diff #181797)

Can we use make_shared here?

source/API/SBType.cpp
107–108 ↗(On Diff #181797)

TypeImpl::GetCompilerType needs to return an Optional also, otherwise you will have the same issue here with zero-size types.

source/Commands/CommandObjectMemory.cpp
527–528 ↗(On Diff #181797)

Can you remove auto here? It's definitely not obvious that it's returning an Optional here, so I originally read this as "if the size is 0, return an error".

650 ↗(On Diff #181797)

Same, please remove auto.

1072 ↗(On Diff #181797)

auto

source/Core/Value.cpp
227–229 ↗(On Diff #181797)

If the ast type is valid (if-check just above), then is it actually possible for this method to fail?

349–351 ↗(On Diff #181797)

Same here. Can this actually fail?

source/Core/ValueObject.cpp
759–762 ↗(On Diff #181797)

auto

827–828 ↗(On Diff #181797)

auto

1826–1828 ↗(On Diff #181797)

auto

1829–1831 ↗(On Diff #181797)

This should probably be auto synthetic_child = std::make_shared<ValueObjectChild>(...);

zturner added inline comments.Jan 15 2019, 11:11 AM
include/lldb/Target/ProcessStructReader.h
70 ↗(On Diff #181797)

I actually changed my mind here. I didn't feel strongly at first because I thought this was a uint64_t. As I read through more of the patch, I realized it was an Optional<uint64_t>, which totally changes the semantics. So now I do feel more strongly

shafik added a subscriber: shafik.Jan 15 2019, 12:16 PM

Sorry for the late review

lldb/trunk/include/lldb/Target/ProcessStructReader.h
70

I will also second the sentiment on the auto, it does not improve readability.

lldb/trunk/source/API/SBType.cpp
106

The early exit is more readable here and also has a lower mental load.

lldb/trunk/source/Commands/CommandObjectMemory.cpp
647

This does not appear to be NFC

1069

NFC?

lldb/trunk/source/Core/Value.cpp
227

This might read better as two steps, get the value and then check it.

lldb/trunk/source/Expression/Materializer.cpp
50

What value does m_size have otherwise?

800

NFC?

lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp
2372

switch?

lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
827

Looks like you chopped out the if (compiler_type) check

lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
1720

There are a lot of checks for base_byte_size so it is not clear what value vfp_byte_size will have if base_byte_size is empty.

The flow looks a lot different but hard to track what will be set and not set.

1788

make_shared?

lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp
2343

switch?

lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp
379

Looks the if (!compiler_type) was removed.

lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp
228

Unrelated formatting changes?

243

Unrelated formatting changes?

268

Formatting?

aprantl marked an inline comment as done.Jan 15 2019, 12:55 PM

I changed all the autos back to full types in r351237.

lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp
827

That's intentional since GetByteSize also checks for IsValid().