Summary: | objc_msgSend and IMP should be cast appropriately before using | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||||||||||
Component: | Platform | Assignee: | Pratik Solanki <psolanki> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | andersca, ap, benjamin, ddkilzer, dglazkov, jberlin, psolanki, webkit.review.bot | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Pratik Solanki
2012-08-28 14:08:42 PDT
Created attachment 161060 [details]
Patch
Comment on attachment 161060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161060&action=review > Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:87 > - return objc_msgSend(delegate, selector, self); > + return reinterpret_cast<id (*)(id, SEL, id)>(objc_msgSend)(delegate, selector, self); What about making a bunch of webkit_objc_msgSend like this?: template<typename Arg1Type, typename Arg2Type> id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1, Arg2Type arg2) { return reinterpret_cast<id (*)(id, SEL, Arg1Type, Arg2Type)>(objc_msgSend)(target, selector, arg1, arg2); } Maybe we should also modify the style checker to prevent bare use of objc_msgSend? Comment on attachment 161060 [details]
Patch
Clearing the review flag, Pratik has an update.
Created attachment 161264 [details]
Patch
(In reply to comment #3) > (From update of attachment 161060 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161060&action=review > > > Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:87 > > - return objc_msgSend(delegate, selector, self); > > + return reinterpret_cast<id (*)(id, SEL, id)>(objc_msgSend)(delegate, selector, self); > > What about making a bunch of webkit_objc_msgSend like this?: > template<typename Arg1Type, typename Arg2Type> > id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1, Arg2Type arg2) > { > return reinterpret_cast<id (*)(id, SEL, Arg1Type, Arg2Type)>(objc_msgSend)(target, selector, arg1, arg2); > } This was a great idea. New patch uploaded with this template code. Comment on attachment 161264 [details]
Patch
r=me
Comment on attachment 161264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161264&action=review > Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:86 > > +template<typename Arg1Type> > +id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1) > +{ > + return reinterpret_cast<id (*)(id, SEL, Arg1Type)>(objc_msgSend)(target, selector, arg1); > +} I would put the templates in a new header in WTF. Then I would ban the use of objc_msgSend through the style checker. I am afraid new code with objc_msgSend might come without the reviewer knowing about this change. This template is limited in what it can do - specifically, it assumes that the functions returns an id. Comment on attachment 161264 [details] Patch Attachment 161264 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13686455 New failing tests: CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread How did this fail? Searching for CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread on the log linked to above, I see [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate [ RUN ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread [ OK ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread (120 ms) [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate (120 ms total) What failed? (In reply to comment #12) > How did this fail? Searching for CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread on the log linked to above, I see > > [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate > [ RUN ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread > [ OK ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread (120 ms) > [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate (120 ms total) > > What failed? It is likely a sick bot. You patch does not touch anything on Linux. Created attachment 161524 [details]
Patch
Comment on attachment 161524 [details]
Patch
Very nice!
Committed r127193: <http://trac.webkit.org/changeset/127193> Looks like llvm-gcc doesn't like my patch. From <http://webkit-commit-queue.appspot.com/results/13724238> In file included from /Volumes/Data/WebKit/Source/WebCore/page/mac/EventHandlerMac.mm:53: /Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:29: error: default template arguments may not be used in function templates /Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:35: error: no default argument for 'Arg1Type' /Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:35: error: default template arguments may not be used in function templates .... and more such errors like this. I'm looking into it. Ok. I can reproduce the build failure by switching to llvm-gcc-4.2 on Mountain Lion (and removing a few -W arguments to the compiler). The template format is template<typename RetType = id, typename Arg1Type> RetType wtfObjcMsgSend(id target, SEL selector, Arg1Type arg1) { return reinterpret_cast<RetType (*)(id, SEL, Arg1Type)>(objc_msgSend)(target, selector, arg1); } If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain? > If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain?
I am afraid we have to support the old compilers.
If the EWS have an old SDK, chances are many people do too.
I think the best is add an explanation in a FIXME and remove the default arg :( :(
(In reply to comment #19) > > If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain? > > I am afraid we have to support the old compilers. > If the EWS have an old SDK, chances are many people do too. > > I think the best is add an explanation in a FIXME and remove the default arg :( :( I agree with Benjamin. Please either do that soon or roll out http://trac.webkit.org/changeset/127193 (it is causing the mac-ews queue to get clogged). Working on this now. I should have a patch soon. Reopening to attach new patch. Created attachment 161752 [details]
Fix for llvm-gcc
Comment on attachment 161752 [details]
Fix for llvm-gcc
Looks good.
I think we should land this so we can take the EWS back online for everyone. Then we fix the EWS and we can eventually revert the patch.
Committed r127306: <http://trac.webkit.org/changeset/127306> Reopening to attach new patch. Created attachment 161762 [details]
Patch
Comment on attachment 161762 [details]
Patch
Damn GCC.
Committed r127313: <http://trac.webkit.org/changeset/127313> |