diff mbox

Fix for libobjc/48177. Can I apply it to 4.6 as well ?

Message ID 1306273309.79029136@www2.webmail.us
State New
Headers show

Commit Message

Nicola Pero May 24, 2011, 9:41 p.m. UTC 
This patch fixes libobjc/48177.  I applied it to trunk.

I'd like to apply this patch to the 4.6 branch too.  Do I need permission from 
a Release Manager ?  Can I get it ? :-)

It is a fairly obvious fix as selector types should generally be compared using
sel_types_match() and not strcmp().

But note that this bug has been in libobjc forever; it never mattered much because the traditional
API (the only one available before GCC 4.6) had a number of different functions for dealing with typed
selectors, and gnustep-base (the main user of this API) was using some other function to do this
and the problem wasn't visible.

The modern API (recommended from GCC 4.6.0 onwards) has a more limited, and elegant, set of functions,
and the code in gnustep-base would be massively simplified ... if only this bug wasn't in the way! :-(

Due to this bug, gnustep-base actually has to forcefully use the traditional API in the middle
of the modern API, which it does by copying some declarations from the traditional API and
it's all quite horrible.  Ideally, we'd fix this bug in trunk and in 4.6.1, so that once 4.6.1
is released, gnustep-base can remove the horrible hacks, simply use the Modern API, and just
tell everyone running 4.6.0 to upgrade to 4.6.1 to get the fix. :-)

So, OK to commit to the 4.6 branch too ?

Thanks

Comments

Mike Stump May 25, 2011, 12:39 a.m. UTC | #1 
On May 24, 2011, at 2:41 PM, Nicola Pero wrote:
> This patch fixes libobjc/48177.  I applied it to trunk.
> 
> I'd like to apply this patch to the 4.6 branch too.  Do I need permission from 
> a Release Manager ?

They are always welcome to chime in, though, in this case the libobjc maintainer can approve it.  For hard to get wrong things that kill desirable features in obscure corners, such a change is fine.  If you wanted to make substantive changes to the register allocator to get 0.001% better on spec, well, that would not fly.  You'd want to be extra careful to not break the build, even with trivial changes, as objc is built by default.  Generally, we manage this by checking into trunk, and letting it bake for a week while people built it.  All that said, if you break the build or create regressions, the RMs reserve the right to whack you with a dead fish.

> The modern API (recommended from GCC 4.6.0 onwards) has a more limited, and elegant, set of functions,
> and the code in gnustep-base would be massively simplified ... if only this bug wasn't in the way! :-(

Fixing important bugs in the only user of the library, would be a really nice thing.

> So, OK to commit to the 4.6 branch too ?

Ok.  :-)
Nicola Pero May 25, 2011, 9:08 a.m. UTC | #2 
>> This patch fixes libobjc/48177.  I applied it to trunk.
>> 
>> I'd like to apply this patch to the 4.6 branch too.  Do I need permission from 
>> a Release Manager ?
>
> They are always welcome to chime in, though, in this case the libobjc maintainer can approve it.

Thanks Mike

I browsed the archives of gcc-patches for a while and as far as I can see, you are right
and other maintainers do approve patches for the 4.6 branch (in their own areas) without
waiting for a Release Manager to double-approve each patch (and it makes sense).

So I applied it to the 4.6 branch too. :-)

Thanks
diff mbox

Patch

Index: libobjc/ChangeLog
===================================================================
--- libobjc/ChangeLog   (revision 174141)
+++ libobjc/ChangeLog   (working copy)
@@ -1,3 +1,10 @@ 
+2011-05-24  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       PR libobjc/48177
+       * selector.c (__sel_register_typed_name): Use sel_types_match()
+       instead of strcmp() to compare selector types (Suggestion by
+       Richard Frith-Macdonald <rfm@gnu.org>).
+
 2011-04-15  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
 
        PR libobjc/32037
Index: libobjc/selector.c
===================================================================
--- libobjc/selector.c  (revision 174138)
+++ libobjc/selector.c  (working copy)
@@ -597,7 +597,7 @@  __sel_register_typed_name (const char *name, const
                    return s;
                }
            }
-         else if (! strcmp (s->sel_types, types))
+         else if (sel_types_match (s->sel_types, types))
            {
              if (orig)
                {
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog     (revision 174142)
+++ gcc/testsuite/ChangeLog     (working copy)
@@ -1,5 +1,10 @@ 
 2011-05-24  Nicola Pero  <nicola.pero@meta-innovation.com>
 
+       PR libobjc/48177
+       * objc.dg/pr48177.m: New testcase.
+
+2011-05-24  Nicola Pero  <nicola.pero@meta-innovation.com>
+
        PR objc/48187
        * objc.dg/pr48187.m: New testcase.
        * obj-c++.dg/pr48187.mm: New testcase.
Index: gcc/testsuite/objc.dg/pr48177.m
===================================================================
--- gcc/testsuite/objc.dg/pr48177.m     (revision 0)
+++ gcc/testsuite/objc.dg/pr48177.m     (revision 0)
@@ -0,0 +1,35 @@ 
+/* Contributed by Nicola Pero <nicola.pero@meta-innovation.com>, May 2011.  */
+/* { dg-do run } */
+/* { dg-skip-if "No API#2 pre-Darwin9" { *-*-darwin[5-8]* } { "-fnext-runtime" } { "" } } */
+
+#include <objc/runtime.h>
+#include <stdlib.h>
+
+int main(int argc, void **args)
+{
+#ifdef __GNU_LIBOBJC__
+  /* This special test tests that, if you have a selector already
+     registered in the runtime with full type information, you can use
+     sel_registerTypedName() to get it even if you specify the type
+     with incorrect argframe information.  This is helpful as
+     selectors generated by the compiler (which have correct argframe
+     information) are usually registered before hand-written ones
+     (which often have incorrect argframe information, but need the
+     correct one).
+
+     Note that in this hand-written test, even the type information of
+     the first selector may be wrong (on this machine); but that's OK
+     as we'll never actually use the selectors.  */
+  SEL selector1 = sel_registerTypedName ("testMethod", "i8@0:4");
+  SEL selector2 = sel_registerTypedName ("testMethod", "i8@8:8");
+  
+  /* We compare the selectors using ==, not using sel_isEqual().  This
+     is because we are testing internals of the runtime and we know
+     that in the current implementation they should be identical if
+     the stuff is to work as expected.  Don't do this at home.  */
+  if (selector1 != selector2)
+    abort ();
+#endif
+
+  return 0;
+}