Page MenuHomePhabricator

Fix places where the return type of a FunctionDecl was being used in place of the function type
ClosedPublic

Authored by bobsayshilol on Oct 14 2018, 3:20 PM.

Details

Summary

FunctionDecl::Create() takes as its T parameter the type of function that should be created, not the return type. Passing in the return type looks to have been copypasta'd around a bit, but the number of correct usages outweighs the incorrect ones so I've opted for keeping what T is the same and fixing up the call sites instead.

This fixes a crash in Clang when attempting to compile the following snippet of code with -fblocks -fsanitize=function -x objective-c++ (my original repro case):

void g(void(^)());
void f()
{
  __block int a = 0;
  g(^(){ a++; });
}

as well as the following which only requires -fsanitize=function -x c++:

void f(char * buf)
{
  __builtin_os_log_format(buf, "");
}

Diff Detail

Repository
rL LLVM

Event Timeline

bobsayshilol created this revision.Oct 14 2018, 3:20 PM
bobsayshilol edited the summary of this revision. (Show Details)

Fixed the added assert to do the right thing. Clang can now build with a debug Clang built from this patch, and all unittests I could find pass in that built version too.

bobsayshilol edited the summary of this revision. (Show Details)Oct 18 2018, 5:46 PM
bobsayshilol retitled this revision from [CodeGen] Fix places where the return type of a FunctionDecl was being used in place of the function type to Fix places where the return type of a FunctionDecl was being used in place of the function type.

Add missing part of previous change to patch. Now all unit tests pass.

I'm removing [CodeGen] from the title too since this change touches more than just that section now.

rjmccall accepted this revision.Nov 8 2018, 7:21 PM

LGTM.

This revision is now accepted and ready to land.Nov 8 2018, 7:21 PM

Thanks!
I don't have commit access to land this myself.

Thanks!
I don't have commit access to land this myself.

I can do it if you'd like, will be a moment though.

I can do it if you'd like, will be a moment though.

Thanks and much appreciated!

I can do it if you'd like, will be a moment though.

Thanks and much appreciated!

Huge apologies, it seems I can't get this to patch cleanly against my fork and therefore can't test it before committing, which is something I generally always do. I'll leave it to someone else. Again, huge apologies, hopefully you won't have to wait too long.

JDevlieghere added a subscriber: JDevlieghere.EditedNov 10 2018, 4:42 PM

The patch applies for me but has quite a few style violations. I'll fix those up before landing it. Also needs a test (I'll add the one from the description).

This revision was automatically updated to reflect the committed changes.

The patch applies for me but has quite a few style violations. I'll fix those up before landing it.

Thank you and sorry for the trouble, my fork seems to have enough modifications to some of these files to confuse patch and getting an untainted checkout and integrating it for a single build/test run would be troublesome to undo.

The patch applies for me but has quite a few style violations. I'll fix those up before landing it.

Thank you and sorry for the trouble, my fork seems to have enough modifications to some of these files to confuse patch and getting an untainted checkout and integrating it for a single build/test run would be troublesome to undo.

No worries! I usually have a pretty up-to-date checkout around (I have cronjob that pulls and builds overnight) which comes in handy in situations like this.

Huge apologies, it seems I can't get this to patch cleanly against my fork and therefore can't test it before committing, which is something I generally always do. I'll leave it to someone else. Again, huge apologies, hopefully you won't have to wait too long.

No worries! Thanks anyway for taking a look.

The patch applies for me but has quite a few style violations. I'll fix those up before landing it. Also needs a test (I'll add the one from the description).

Ah I was trying to match the code around it and didn't think to run clang-tidy so I'll keep that in mind next time, and I didn't add a new test because just the assert in Decl.cpp was enough to flag up misuses with check-clang but I'm definitely not against adding more tests. Thanks for landing!

Thanks again everyone!