Page MenuHomePhabricator

Loop pragma parsing. NFC.

Authored by SjoerdMeijer on Jul 11 2019, 6:14 AM.



Yesterday I was a bit distracted by loop pragma parsing (D64471), but still am a bit today too (this patch).

Event Timeline

SjoerdMeijer created this revision.Jul 11 2019, 6:14 AM
SjoerdMeijer marked an inline comment as done.Jul 11 2019, 6:17 AM
SjoerdMeijer added inline comments.
1015 ↗(On Diff #215162)

I guess this does behave slightly different, i.e. the assert in a release build.

Before, in a release build, "unroll" was returned for an unexpected pragma, but now we return the empty string "".

Meinersbur added inline comments.Jul 11 2019, 9:43 AM
1009–1016 ↗(On Diff #215162)

getName() returns StringRef. No need to use a std::string yet.

1011–1014 ↗(On Diff #215162)

This unconditionally creates (at least) 5 std::string objects.

1040–1045 ↗(On Diff #215162)

[style] We don't use const for local variables. Could also use auto here since the type is already explicit on the RHS.

[suggestion] IsLoopHint would indicate the meaning of the boolean variable.

SjoerdMeijer marked an inline comment as not done.

thanks for taking a look and the suggestions!

This comment was removed by SjoerdMeijer.
This revision is now accepted and ready to land.Jul 30 2019, 8:34 AM
Meinersbur requested changes to this revision.Jul 30 2019, 8:44 AM
Meinersbur added inline comments.
1011 ↗(On Diff #209299)

[serious] I know I already accepted the patch, but I just noticed something:
"clang loop " + Str.str() will allocate a temporary std::string, Str will potentially point to it, then the temporary string will be released. Str will then point to released memory and returned by this function, i.e. a use-after-free.

This revision now requires changes to proceed.Jul 30 2019, 8:44 AM
SjoerdMeijer marked an inline comment as done.Jul 30 2019, 11:13 AM
SjoerdMeijer added inline comments.
1011 ↗(On Diff #209299)

Oopsy, thanks for spotting this! Will fix this!

Fixed the string problems.

this is not important at all, but might be nice clean up, so friendly ping :-)

Meinersbur added inline comments.Aug 13 2019, 11:42 AM
1010 ↗(On Diff #215162)

[serious] Use-after-free here again. This line will do the following:

StringRef ClangLoopStr;
std::string tmp = "clang loop " + Str.str()
ClangLoopStr = tmp;
// tmp.~string() 
// Any use of ClangLoopStr will use memory released by tmp.~string()

Let me suggest a solution:

std::string ClangLoopStr = (Twine("clang loop ") + Str).str();
std::string Result = llvm::StringSwitch<StringRef>(Str)
               .Case("loop", ClangLoopStr)
               .Case("unroll_and_jam", Str)
               .Case("unroll", Str)
return Result; // NRVO, ClangLoopStr will be released here, but if it was chosen by the StringSwitch, Result will hold a copy, so ClangLoopStr is not referenced anymore.

Note that this will alloc one more std::string in the non-ClangLoopStr cases than before the patch, but I don't think it's important.

Sorry, should have done my std::string, StringRef, and Twine homework a lot better!

Thanks for your help and suggestions, will fix this.

thanks for the suggestions; comments addressed.

Meinersbur accepted this revision.Aug 14 2019, 1:44 PM
Meinersbur added inline comments.
1010 ↗(On Diff #215162)

[nit] I am surprised it works without llvm:: qualifier for llvm::Twine. Maybe add it for consistency?

1016 ↗(On Diff #215162)

[nit] One could just return llvm::StringSwitch... without a Result variable. I used it in the suggestion so I could comment on what happens when returning. Personally, there is nothing that I could care less about, so use what you prefer.

This revision is now accepted and ready to land.Aug 14 2019, 1:44 PM

Thanks, and will fix your suggestions before committing.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2019, 12:40 AM