r213265 - Using a std::string instead of a StringRef because the Default case synthesizes a temporary std::string from a Twine. Assigning that into a StringRef causes the StringRef to refer to a temporary, and bad things happen.

[Daniel Sanders]

Ah, I understand now. Thanks.

> -----Original Message-----
> From: aaron.ballman at gmail.com [mailto:aaron.ballman at gmail.com] On
> Behalf Of Aaron Ballman
> Sent: 17 July 2014 15:51
> To: Daniel Sanders
> Cc: Alp Toker; llvm cfe
> Subject: Re: r213265 - Using a std::string instead of a StringRef because the
> Default case synthesizes a temporary std::string from a Twine. Assigning that
> into a StringRef causes the StringRef to refer to a temporary, and bad things
> happen.
> 
> On Thu, Jul 17, 2014 at 10:39 AM, Daniel Sanders
> <Daniel.Sanders at imgtec.com> wrote:
> > Thanks for fixing this. It also caused some other buildbot failures which I'm
> in the middle of debugging.
> >
> > It's my understanding that the Args.MakeArgString() call a few lines further
> down is supposed to convert the temporary string into a string with the same
> lifetime as the argument list. That's clearly not happening so I'm wondering if
> there's a possible bug there. Have I misunderstood MakeArgString()?
> 
> No, MarkArgString() turns out to be immaterial. The problem is that ("+" +
> ABIName).str() results in a std::string temporary object whose lifetime is that
> of the full expression containing it. That resulting StringRef (from the
> StringSwitch call chain) is referencing data which is then destroyed after the
> assignment takes place.
> 
> ~Aaron
> 
> >
> >> -----Original Message-----
> >> From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-
> >> bounces at cs.uiuc.edu] On Behalf Of Aaron Ballman
> >> Sent: 17 July 2014 15:09
> >> To: Alp Toker
> >> Cc: llvm cfe
> >> Subject: Re: r213265 - Using a std::string instead of a StringRef
> >> because the Default case synthesizes a temporary std::string from a
> >> Twine. Assigning that into a StringRef causes the StringRef to refer
> >> to a temporary, and bad things happen.
> >>
> >> On Thu, Jul 17, 2014 at 10:03 AM, Alp Toker <alp at nuanti.com> wrote:
> >> >
> >> > On 17/07/2014 16:28, Aaron Ballman wrote:
> >> >>
> >> >> Author: aaronballman
> >> >> Date: Thu Jul 17 08:28:50 2014
> >> >> New Revision: 213265
> >> >>
> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=213265&view=rev
> >> >> Log:
> >> >> Using a std::string instead of a StringRef because the Default
> >> >> case synthesizes a temporary std::string from a Twine. Assigning
> >> >> that into a StringRef causes the StringRef to refer to a
> >> >> temporary, and bad things happen.
> >> >>
> >> >> This fixes a failing test case on Windows.
> >> >>
> >> >> Modified:
> >> >>      cfe/trunk/lib/Driver/Tools.cpp
> >> >>
> >> >> Modified: cfe/trunk/lib/Driver/Tools.cpp
> >> >> URL:
> >> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp
> >> >> ?re v=213265&r1=213264&r2=213265&view=diff
> >> >>
> >> >>
> >>
> ==========================================================
> >> ===========
> >> >> =========
> >> >> --- cfe/trunk/lib/Driver/Tools.cpp (original)
> >> >> +++ cfe/trunk/lib/Driver/Tools.cpp Thu Jul 17 08:28:50 2014
> >> >> @@ -1037,12 +1037,12 @@ static void getMIPSTargetFeatures(const
> >> >>     ABIName = getGnuCompatibleMipsABIName(ABIName);
> >> >>       // Always override the backend's default ABI.
> >> >> -  StringRef ABIFeature = llvm::StringSwitch<StringRef>(ABIName)
> >> >> -                              .Case("32", "+o32")
> >> >> -                              .Case("n32", "+n32")
> >> >> -                              .Case("64", "+n64")
> >> >> -                              .Case("eabi", "+eabi")
> >> >> -                              .Default(("+" + ABIName).str());
> >> >> +  std::string ABIFeature = llvm::StringSwitch<StringRef>(ABIName)
> >> >> +                               .Case("32", "+o32")
> >> >> +                               .Case("n32", "+n32")
> >> >> +                               .Case("64", "+n64")
> >> >> +                               .Case("eabi", "+eabi")
> >> >> +                               .Default(("+" + ABIName).str());
> >> >
> >> >
> >> > Is the optimizer smart enough to eliminate the evaluation of that
> >> > str() call or will the string be built and discarded every time the
> >> > StringSwitch is run?
> >>
> >> I'm not certain (I've not tested it), but I would guess the str()
> >> call is evaluated each time because it is used as an argument in a
> >> function call which is evaluated.
> >>
> >> ~Aaron
> >> _______________________________________________
> >> cfe-commits mailing list
> >> cfe-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits