[llvm] r218514 - Revert "Store TypeUnits in a SmallVector<DWARFUnitSection> instead of a single DWARFUnitSection."

Frédéric Riss friss at apple.com
Fri Sep 26 23:07:34 PDT 2014


> On 26 Sep 2014, at 23:39, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On Sep 26, 2014, at 6:49 AM, Frédéric Riss <friss at apple.com> wrote:
>> 
>>> 
>>> On 26 Sep 2014, at 14:34, Frederic Riss <friss at apple.com> wrote:
>>> 
>>> Author: friss
>>> Date: Fri Sep 26 07:34:06 2014
>>> New Revision: 218514
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=218514&view=rev
>>> Log:
>>> Revert "Store TypeUnits in a SmallVector<DWARFUnitSection> instead of a single DWARFUnitSection."
>>> 
>>> This reverts commit r218513.
>>> 
>>> Buildbots using libstdc++ issue an error when trying to copy
>>> SmallVector<std::unique_ptr<>>. Revert the commit until we have a fix.
>> 
>> This wasn’t only libstdc++ in fact, MSVC also chocked and I have one libcxx bot that also failed.
>> 
>> David, if you look at the commit, you’ll see that I didn’t replace the SmallVectors we talked about by std::vectors. I tried it, but it failed in a very similar way to the builders. I thought this was some libcxx or SmallVector issue and I was going to report it with a reproducer:
>> 
>> #include <vector>
>> #include <memory>
>> #include <llvm/ADT/SmallVector.h>
>> 
>> typedef llvm::SmallVector<std::unique_ptr<int>,1> vec_type;
>> std::vector<vec_type> vecvec;
>> 
>> void foo() {
>> 	vecvec.push_back(vec_type());
>> }
>> 
>> Is this code somehow wrong?
>> This fails to compile for me because the move semantics are lost somewhere and the implicitly deleted unique_ptr copy constructor is called. Note that other permutations of vector and SmallVector seem to work.
>> 
>> If I add explicit default and move constructors to DWARFUnitSection, then I am able to replace the SmallVectors by vectors as we discussed, but I don’t understand why this is necessary. Maybe this would also fix the issue seen on the builders, but in either case I can’t see why the compiler generated constructors don’t work. 
>> 
>> For the record, here are some builder logs:
>> http://lab.llvm.org:8011/builders/clang-native-arm-cortex-a9/builds/21968/steps/compile/logs/stdio
> 
> I looked at this one.  It looks to me like the compiler hasn’t generated
> a move constructor for `DWARFUnitSection`.  I knew MSVC had issues like
> this, but maybe GCC has trouble to.  We can’t really trust compilers to
> auto-generate C++11 constructors properly.

By ‘compilers’ you mean clang too ? because there was one clang++ failure in the logs I provided (on freebsd). I suppose it can be an old version that had the same kind of issues.

> Looking at the code, you probably just need something like this:
> 
>     DwarfUnitSection(DwarfUnitSection &&X)
>         : SmallVector<std::unique_ptr<UnitType>>(std::move(X)) {}

Yeah, and also add an empty default constructor. I just wanted to make sure that the code in itself wasn’t flawed in a way that I didn’t understand.

Thanks for looking into it!
Fred

> If MSVC supported it, you could do this instead:
> 
>     DwarfUnitSection(DwarfUnitSection &&) = default;
> 
>> http://lab.llvm.org:8011/builders/lld-x86_64-freebsd/builds/8474/steps/build_Lld/logs/stdio
>> http://lab.llvm.org:8011/builders/lld-x86_64-win7/builds/13282/steps/build_Lld/logs/stdio
>> 
>> Fred
>> 
>>> Modified:
>>>   llvm/trunk/lib/DebugInfo/DWARFContext.cpp
>>>   llvm/trunk/lib/DebugInfo/DWARFContext.h
>>> 
>>> Modified: llvm/trunk/lib/DebugInfo/DWARFContext.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFContext.cpp?rev=218514&r1=218513&r2=218514&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/DebugInfo/DWARFContext.cpp (original)
>>> +++ llvm/trunk/lib/DebugInfo/DWARFContext.cpp Fri Sep 26 07:34:06 2014
>>> @@ -86,17 +86,15 @@ void DWARFContext::dump(raw_ostream &OS,
>>> 
>>>  if ((DumpType == DIDT_All || DumpType == DIDT_Types) && getNumTypeUnits()) {
>>>    OS << "\n.debug_types contents:\n";
>>> -    for (const auto &TUS : type_unit_sections())
>>> -      for (const auto &TU : TUS)
>>> -        TU->dump(OS);
>>> +    for (const auto &TU : type_units())
>>> +      TU->dump(OS);
>>>  }
>>> 
>>>  if ((DumpType == DIDT_All || DumpType == DIDT_TypesDwo) &&
>>>      getNumDWOTypeUnits()) {
>>>    OS << "\n.debug_types.dwo contents:\n";
>>> -    for (const auto &DWOTUS : dwo_type_unit_sections())
>>> -      for (const auto &DWOTU : DWOTUS)
>>> -        DWOTU->dump(OS);
>>> +    for (const auto &DWOTU : dwo_type_units())
>>> +      DWOTU->dump(OS);
>>>  }
>>> 
>>>  if (DumpType == DIDT_All || DumpType == DIDT_Loc) {
>>> @@ -339,17 +337,15 @@ void DWARFContext::parseTypeUnits() {
>>>    uint32_t offset = 0;
>>>    const DataExtractor &DIData =
>>>        DataExtractor(I.second.Data, isLittleEndian(), 0);
>>> -    TUs.push_back(DWARFUnitSection<DWARFTypeUnit>());
>>> -    auto &TUS = TUs.back();
>>>    while (DIData.isValidOffset(offset)) {
>>>      std::unique_ptr<DWARFTypeUnit> TU(new DWARFTypeUnit(*this,
>>>           getDebugAbbrev(), I.second.Data, getRangeSection(),
>>>           getStringSection(), StringRef(), getAddrSection(),
>>> -           &I.second.Relocs, isLittleEndian(), TUS));
>>> +           &I.second.Relocs, isLittleEndian(), TUs));
>>>      if (!TU->extract(DIData, &offset))
>>>        break;
>>> -      TUS.push_back(std::move(TU));
>>> -      offset = TUS.back()->getNextUnitOffset();
>>> +      TUs.push_back(std::move(TU));
>>> +      offset = TUs.back()->getNextUnitOffset();
>>>    }
>>>  }
>>> }
>>> @@ -380,17 +376,15 @@ void DWARFContext::parseDWOTypeUnits() {
>>>    uint32_t offset = 0;
>>>    const DataExtractor &DIData =
>>>        DataExtractor(I.second.Data, isLittleEndian(), 0);
>>> -    DWOTUs.push_back(DWARFUnitSection<DWARFTypeUnit>());
>>> -    auto &TUS = DWOTUs.back();
>>>    while (DIData.isValidOffset(offset)) {
>>>      std::unique_ptr<DWARFTypeUnit> TU(new DWARFTypeUnit(*this,
>>>          getDebugAbbrevDWO(), I.second.Data, getRangeDWOSection(),
>>>          getStringDWOSection(), getStringOffsetDWOSection(), getAddrSection(),
>>> -          &I.second.Relocs, isLittleEndian(), TUS));
>>> +          &I.second.Relocs, isLittleEndian(), DWOTUs));
>>>      if (!TU->extract(DIData, &offset))
>>>        break;
>>> -      TUS.push_back(std::move(TU));
>>> -      offset = TUS.back()->getNextUnitOffset();
>>> +      DWOTUs.push_back(std::move(TU));
>>> +      offset = DWOTUs.back()->getNextUnitOffset();
>>>    }
>>>  }
>>> }
>>> 
>>> Modified: llvm/trunk/lib/DebugInfo/DWARFContext.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/DebugInfo/DWARFContext.h?rev=218514&r1=218513&r2=218514&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/DebugInfo/DWARFContext.h (original)
>>> +++ llvm/trunk/lib/DebugInfo/DWARFContext.h Fri Sep 26 07:34:06 2014
>>> @@ -30,7 +30,7 @@ namespace llvm {
>>> class DWARFContext : public DIContext {
>>> 
>>>  DWARFUnitSection<DWARFCompileUnit> CUs;
>>> -  SmallVector<DWARFUnitSection<DWARFTypeUnit>,1> TUs;
>>> +  DWARFUnitSection<DWARFTypeUnit> TUs;
>>>  std::unique_ptr<DWARFDebugAbbrev> Abbrev;
>>>  std::unique_ptr<DWARFDebugLoc> Loc;
>>>  std::unique_ptr<DWARFDebugAranges> Aranges;
>>> @@ -38,7 +38,7 @@ class DWARFContext : public DIContext {
>>>  std::unique_ptr<DWARFDebugFrame> DebugFrame;
>>> 
>>>  DWARFUnitSection<DWARFCompileUnit> DWOCUs;
>>> -  SmallVector<DWARFUnitSection<DWARFTypeUnit>,1> DWOTUs;
>>> +  DWARFUnitSection<DWARFTypeUnit> DWOTUs;
>>>  std::unique_ptr<DWARFDebugAbbrev> AbbrevDWO;
>>>  std::unique_ptr<DWARFDebugLocDWO> LocDWO;
>>> 
>>> @@ -77,7 +77,6 @@ public:
>>> 
>>>  typedef DWARFUnitSection<DWARFCompileUnit>::iterator_range cu_iterator_range;
>>>  typedef DWARFUnitSection<DWARFTypeUnit>::iterator_range tu_iterator_range;
>>> -  typedef iterator_range<SmallVectorImpl<DWARFUnitSection<DWARFTypeUnit>>::iterator> tu_section_iterator_range;
>>> 
>>>  /// Get compile units in this context.
>>>  cu_iterator_range compile_units() {
>>> @@ -86,9 +85,9 @@ public:
>>>  }
>>> 
>>>  /// Get type units in this context.
>>> -  tu_section_iterator_range type_unit_sections() {
>>> +  tu_iterator_range type_units() {
>>>    parseTypeUnits();
>>> -    return tu_section_iterator_range(TUs.begin(), TUs.end());
>>> +    return tu_iterator_range(TUs.begin(), TUs.end());
>>>  }
>>> 
>>>  /// Get compile units in the DWO context.
>>> @@ -98,9 +97,9 @@ public:
>>>  }
>>> 
>>>  /// Get type units in the DWO context.
>>> -  tu_section_iterator_range dwo_type_unit_sections() {
>>> +  tu_iterator_range dwo_type_units() {
>>>    parseDWOTypeUnits();
>>> -    return tu_section_iterator_range(DWOTUs.begin(), DWOTUs.end());
>>> +    return tu_iterator_range(DWOTUs.begin(), DWOTUs.end());
>>>  }
>>> 
>>>  /// Get the number of compile units in this context.
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140927/e8653e8f/attachment.html>


More information about the llvm-commits mailing list