[llvm] r218514 - Revert "Store TypeUnits in a SmallVector<DWARFUnitSection> instead of a single DWARFUnitSection."
David Blaikie
dblaikie at gmail.com
Mon Sep 29 15:02:29 PDT 2014
On Fri, Sep 26, 2014 at 11:07 PM, Frédéric Riss <friss at apple.com> wrote:
>
> 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.
>
Not sure what happened there - I could only guess at attributing that to an
old standard library that didn't have an rvalue overload of
vector::push_back
>
> 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
>
>
>
> _______________________________________________
> 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/20140929/3dc095b8/attachment.html>
More information about the llvm-commits
mailing list