[llvm] r231269 - Revert "unique_ptrify ValID::ConstantStructElts"

[David Blaikie]

On Wed, Mar 4, 2015 at 10:31 AM, Reid Kleckner <reid at kleckner.net> wrote:

> Author: rnk
> Date: Wed Mar  4 12:31:10 2015
> New Revision: 231269
>
> URL: http://llvm.org/viewvc/llvm-project?rev=231269&view=rev
> Log:
> Revert "unique_ptrify ValID::ConstantStructElts"
>
> This reverts r231200 and r231204. The second one added an explicit move
> ctor for MSVC.
>
> This change broke the clang-cl self-host due to weirdness in MSVC's
> implementation of std::map::insert. Somehow we lost our rvalue ref-ness
> when going through variadic placement new:
>
>   template <class _Objty, class... _Types>
>   void construct(_Objty *_Ptr,
>                  _Types &&... _Args) { // construct _Objty(_Types...) at
> _Ptr
>     ::new ((void *)_Ptr) _Objty(_STD forward<_Types>(_Args)...);
>   }
>
> For some reason, Clang decided to call the deleted std::pair copy
> constructor at this point. Needs further investigation, once I can
> build.
>

Investigated this some more - the issue seemed to be a few layers up. The
actual insert function chosen seemed to be the "const value_type&"
overload, not the P&& perfect forwarding template - the latter is SFINAE'd
on "is_convertible" and, as per the standard. Strangely, rvalue of
"std::pair<foo, int>" is not implicitly convertible to std::pair<const foo,
int> - I don't know why. GCC uses is_constructible, rather than
is_convertible but I think that's wrong (you could end up calling explicit
ctors which you shouldn't get from 'insert', you should only get that from
'emplace')

And of course GCC 4.7 doesn't have std::map::emplace... - so, kind of stuck
between a rock & a hard place.

Richard - any thoughts on any of this, or at least: why is
std::pair<move_only, int> not convertible to std::pair<const move_only,
int> ?

I guess I can make this change less intrusive - make ValID copyable and
just assert that it's not the case where it owns memory (ensure the
unique_ptr is null).

- David


>
> Modified:
>     llvm/trunk/lib/AsmParser/LLParser.cpp
>     llvm/trunk/lib/AsmParser/LLParser.h
>
> Modified: llvm/trunk/lib/AsmParser/LLParser.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/LLParser.cpp?rev=231269&r1=231268&r2=231269&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/AsmParser/LLParser.cpp (original)
> +++ llvm/trunk/lib/AsmParser/LLParser.cpp Wed Mar  4 12:31:10 2015
> @@ -2365,10 +2365,9 @@ bool LLParser::ParseValID(ValID &ID, Per
>          ParseToken(lltok::rbrace, "expected end of struct constant"))
>        return true;
>
> -    ID.ConstantStructElts.reset(new Constant *[Elts.size()]);
> +    ID.ConstantStructElts = new Constant*[Elts.size()];
>      ID.UIntVal = Elts.size();
> -    memcpy(ID.ConstantStructElts.get(), Elts.data(),
> -           Elts.size() * sizeof(Elts[0]));
> +    memcpy(ID.ConstantStructElts, Elts.data(),
> Elts.size()*sizeof(Elts[0]));
>      ID.Kind = ValID::t_ConstantStruct;
>      return false;
>    }
> @@ -2387,9 +2386,8 @@ bool LLParser::ParseValID(ValID &ID, Per
>        return true;
>
>      if (isPackedStruct) {
> -      ID.ConstantStructElts.reset(new Constant *[Elts.size()]);
> -      memcpy(ID.ConstantStructElts.get(), Elts.data(),
> -             Elts.size() * sizeof(Elts[0]));
> +      ID.ConstantStructElts = new Constant*[Elts.size()];
> +      memcpy(ID.ConstantStructElts, Elts.data(),
> Elts.size()*sizeof(Elts[0]));
>        ID.UIntVal = Elts.size();
>        ID.Kind = ValID::t_PackedConstantStruct;
>        return false;
> @@ -3949,8 +3947,8 @@ bool LLParser::ConvertValIDToValue(Type
>            return Error(ID.Loc, "element " + Twine(i) +
>                      " of struct initializer doesn't match struct element
> type");
>
> -      V = ConstantStruct::get(
> -          ST, makeArrayRef(ID.ConstantStructElts.get(), ID.UIntVal));
> +      V = ConstantStruct::get(ST, makeArrayRef(ID.ConstantStructElts,
> +                                               ID.UIntVal));
>      } else
>        return Error(ID.Loc, "constant expression type mismatch");
>      return false;
>
> Modified: llvm/trunk/lib/AsmParser/LLParser.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/AsmParser/LLParser.h?rev=231269&r1=231268&r2=231269&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/AsmParser/LLParser.h (original)
> +++ llvm/trunk/lib/AsmParser/LLParser.h Wed Mar  4 12:31:10 2015
> @@ -62,17 +62,13 @@ namespace llvm {
>      APSInt APSIntVal;
>      APFloat APFloatVal;
>      Constant *ConstantVal;
> -    std::unique_ptr<Constant*[]> ConstantStructElts;
> +    Constant **ConstantStructElts;
>
>      ValID() : Kind(t_LocalID), APFloatVal(0.0) {}
> -    // Workaround for MSVC not synthesizing implicit move members.
> -    ValID(ValID &&RHS)
> -        : Kind(std::move(RHS.Kind)), Loc(std::move(RHS.Loc)),
> -          UIntVal(std::move(RHS.UIntVal)), StrVal(std::move(RHS.StrVal)),
> -          StrVal2(std::move(RHS.StrVal2)),
> APSIntVal(std::move(RHS.APSIntVal)),
> -          APFloatVal(std::move(RHS.APFloatVal)),
> -          ConstantVal(std::move(RHS.ConstantVal)),
> -          ConstantStructElts(std::move(RHS.ConstantStructElts)) {}
> +    ~ValID() {
> +      if (Kind == t_ConstantStruct || Kind == t_PackedConstantStruct)
> +        delete [] ConstantStructElts;
> +    }
>
>      bool operator<(const ValID &RHS) const {
>        if (Kind == t_LocalID || Kind == t_GlobalID)
>
>
> _______________________________________________
> 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/20150308/91453e6c/attachment.html>