[libcxx-commits] [PATCH] D101206: [libc++] Remove UB in list, forward_list and __hash_table

[Michael Buch via Phabricator via libcxx-commits]

Michael137 added a comment.

In D101206#4653387 <https://reviews.llvm.org/D101206#4653387>, @Michael137 wrote:

> In D101206#4653303 <https://reviews.llvm.org/D101206#4653303>, @dblaikie wrote:
>
>> In D101206#4653253 <https://reviews.llvm.org/D101206#4653253>, @Michael137 wrote:
>>
>>> Looks like this broke the LLDB buildbot on Darwin: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/61029/console
>>>
>>> Specifically the formatters:
>>>
>>>   Failed Tests (2):
>>>     lldb-api :: commands/expression/import-std-module/queue/TestQueueFromStdModule.py
>>>     lldb-api :: functionalities/data-formatter/data-formatter-stl/generic/unordered/TestDataFormatterGenericUnordered.py
>>>
>>>   AssertionError: Ran command:
>>>   "frame variable map"
>>>   
>>>   Got output:
>>>   (std::unordered_map<int, std::string>) map = size=5 {}
>>>   
>>>   Expecting regex pattern: "std::unordered_map" (was found, matched "std::unordered_map")
>>>   Expecting regex pattern: "\[0\] = \{\s*first = " (was not found)
>>>
>>> Presumably because we removed the `_LIBCPP_STANDALONE_DEBUG`. I don't think we do constructor homing on Darwin since we do `-fstandalone-debug`
>>
>> Adding/removing the attribute shouldn't make any difference on Darwin. (the standalone debug attribute says "treat this type as though it were compiled with `-fstandalone-debug`" and Darwin's default is `-fstandalone-debug` - so adding/removing the attribute shouldn't change the behavior - it's possible there are bugs here, but I'd wager that's not the case/is a red herring)
>
> Good point, though I did confirm that this patch caused the breakage.
> Reason I pointed it at the attribute was following error:
>
>   error: warning: warning: got name from symbols: C
>   error: Couldn't look up symbols:
>     __ZNSt3__19__voidifyB7v180000INS_11__list_nodeI1CPvEEEES3_RT_
>
> Which on first glance sounds like a "lack of debug info" problem. Though your point makes sense, it shouldn't affect Darwin since it's already the default. Will investigate what else it could be

Ah the problem is most likely because `__value_` is now wrapped in an anonymous union. Which breaks the formatter's assumption:

  // In LibCxxUnorderedMap.cpp
  node_sp = m_next_element->Cast(compiler_type: m_node_type.GetPointerType()) 
          ->Dereference(&: error);                                            
  if (!node_sp || error.Fail())                                               
      return nullptr;                                                         
                                                                              
  value_sp = node_sp->GetChildMemberWithName(name: "__value_");

Should be able to fix this fairly quick. The other test is failing for a different reason by the looks of it. But it is for a much less widely used feature and I don't think it really needs to block this from landing if we don't get to the bottom of it soon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101206/new/

https://reviews.llvm.org/D101206