vsk marked an inline comment as done. vsk added a comment. Thanks everyone for the reviews!
================ Comment at: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:29 + if (sizeof(std::string) == sizeof(garbage_string_sso)) + memcpy((void *)&garbage1, &garbage_string_sso, sizeof(std::string)); + if (sizeof(std::string) == sizeof(garbage_string_long)) ---------------- vsk wrote: > teemperor wrote: > > teemperor wrote: > > > shafik wrote: > > > > vsk wrote: > > > > > shafik wrote: > > > > > > While I get what you are doing here, we know he structure of libc++ > > > > > > SSO implementation and we are manually building a corrupt one, this > > > > > > is fragile to changes in the implementation. > > > > > > > > > > > > I don't have an immediate suggestion for an alternative approach > > > > > > but if we stick with this we should stick a big comment explaining > > > > > > this, perhaps laying out the assumptions of the internal layout we > > > > > > are assuming and maybe some sanity checks maybe using `offsetof` to > > > > > > verify fields exist and are where we expect them to be. > > > > > I don't see how this is fragile. The structure of libc++'s SSO > > > > > implementation is ABI, and is unlikely to change (esp. not in a way > > > > > that turns either one of the garbage strings into a valid string). > > > > > I've left comments explaining what's wrong with both of the garbage > > > > > strings, but can leave a pointer to > > > > > https://joellaity.com/2020/01/31/string.html for more info? > > > > Sure, that note would be fine. > > > Can you instead do a `#if _LIBCPP_ABI_VERSION == 1` and have the #else as > > > an #error that this test needs updating. We don't support any other > > > libc++ ABI beside 1 in LLDB but if we ever do then this should not > > > silently pass. > > Actually the #if *and* a static_assert comparing the size would be best > > IMHO. > Sure, but the size check is not primarily about the ABI. The garbage examples > presuppose 64-bit pointer & size types, which is not true on some watches. I'm not sure how to write a static assert that isn't a little brittle. Maybe `static_assert(sizeof(void *) != 8 || sizeof(std::string) == 24, "unknown std::string layout")`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73860/new/ https://reviews.llvm.org/D73860 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
