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.

[Aaron Ballman]

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