vsk added inline comments.
================ 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)) ---------------- 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. 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. ================ Comment at: lldb/source/DataFormatters/StringPrinter.cpp:149 + if ((utf8_encoded_len == 0 || utf8_encoded_len > 4) || + !isInHalfOpenRange(buffer + (utf8_encoded_len - 1), buffer, buffer_end)) return retval; ---------------- teemperor wrote: > Isnt' `!isInHalfOpenRange(buffer + (utf8_encoded_len - 1), buffer, > buffer_end))` just `buffer + (utf8_encoded_len - 1U) < buffer_end`? > `utf8_encoded_len` is always positive so the check if it adding it to > `buffer` makes it smaller than `buffer` can only happen with an integer > overflow IIUC (which we probably should check against more explicitly then). I've lifted the `buffer < buffer_end` check into `GetPrintable`, and made the overflow check here explicit. 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
