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).

Diff Detail

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 #209194)

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 ↗(On Diff #209194)

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

1011–1014 ↗(On Diff #209194)

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

1040 ↗(On Diff #209194)

[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.

[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.

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 #212878)

[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