[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: [PATCH] objc: Fix handling of break stmt inside of switch inside of ObjC foreach [PR103639]
From:       Iain Sandoe <iain () sandoe ! co ! uk>
Date:       2021-12-31 12:01:00
Message-ID: 7213725E-BB8E-41D5-A1D7-4973DEA40369 () sandoe ! co ! uk
[Download RAW message or body]

Hi Jakub,
thanks for looking at this,

> On 30 Dec 2021, at 09:33, Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> \
> wrote: 

> The r11-3302-g3696a50beeb73f changes broke the following ObjC testcase.
> in_statement is either 0 (not in a looping statement), various IN_* flags
> for various kinds of looping statements (or OpenMP structured blocks) or
> those flags ored with IN_SWITCH_STMT when a switch appears inside of those
> contexts.  This is because break binds to switch in that last case, but
> continue binds to the looping construct in that case.
> The c_finish_bc_stmt function performs diagnostics on incorrect
> break/continue uses and then checks if in_statement & IN_OBJC_FOREACH
> and in that case jumps to the label provided by the caller, otherwise
> emits a BREAK_STMT or CONTINUE_STMT.  This is incorrect if we have
> ObjC foreach with switch nested in it and break inside of that,
> in_statement in that case is IN_OBJC_FOREACH | IN_SWITCH_STMT and
> is_break is true.  We want to handle it like other breaks inside of
> switch, i.e. emit a BREAK_STMT.
> 
> The following patch fixes that.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I regstrapped this on x86_64-darwin9 and i686-darwin9 and checked this
for NeXT and GNU runtimes for m32 and m64 (also on powerpc-darwin9,
but without a bootstrap).

From the Objective-C perspective, LGTM.
thanks
Iain

> 
> 2021-12-30  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR objc/103639
> 	* c-typeck.c (c_finish_bc_stmt): For break inside of switch inside of
> 	ObjC foreach, emit normal BREAK_STMT rather than goto to label.
> 
> 2021-12-30  Iain Sandoe  <iain@sandoe.co.uk>
> 
> 	PR objc/103639
> 	* objc.dg/pr103639.m: New test.
> 
> --- gcc/c/c-typeck.c.jj	2021-12-09 15:37:27.657304583 +0100
> +++ gcc/c/c-typeck.c	2021-12-29 16:27:56.693351501 +0100
> @@ -11257,7 +11257,8 @@ c_finish_bc_stmt (location_t loc, tree l
> 
> if (skip)
> return NULL_TREE;
> -  else if (in_statement & IN_OBJC_FOREACH)
> +  else if ((in_statement & IN_OBJC_FOREACH)
> +	   && !(is_break && (in_statement & IN_SWITCH_STMT)))
> {
> /* The foreach expander produces low-level code using gotos instead
> 	 of a structured loop construct.  */
> --- gcc/testsuite/objc.dg/pr103639.m.jj	2021-11-07 16:02:58.901842074 +0100
> +++ gcc/testsuite/objc.dg/pr103639.m	2021-12-29 21:42:08.253107653 +0100
> @@ -0,0 +1,101 @@
> +/* PR objc/103639 */
> +/* { dg-do run } */
> +/* { dg-skip-if "No NeXT fast enum. pre-Darwin9" { *-*-darwin[5-8]* } { \
> "-fnext-runtime" } { "" } } */ +/* { dg-xfail-run-if "Needs OBJC2 ABI" { \
> *-*-darwin* && { lp64 && { ! objc2 } } } { "-fnext-runtime" } { "" } } */ +/* { \
> dg-additional-sources "../objc-obj-c++-shared/nsconstantstring-class-impl.m" } */ \
> +/* { dg-additional-options "-mno-constant-cfstrings" { target *-*-darwin* } } */ \
> +/* { dg-additional-options "-Wno-objc-root-class" } */ +
> +#import "../objc-obj-c++-shared/TestsuiteObject.m"
> +#ifndef __NEXT_RUNTIME__
> +#include <objc/NXConstStr.h>
> +#else
> +#include "../objc-obj-c++-shared/nsconstantstring-class.h"
> +#endif
> +
> +extern int printf (const char *, ...);
> +#include <stdlib.h>
> +
> +/* A mini-array implementation that can be used to test fast
> +    enumeration.  You create the array with some objects; you can
> +    mutate the array, and you can fast-enumerate it.
> + */
> +@interface MyArray : TestsuiteObject
> +{
> +  unsigned int length;
> +  id *objects;
> +  unsigned long mutated;
> +}
> +- (id) initWithLength: (unsigned int)l  objects: (id *)o;
> +- (void) mutate;
> +- (unsigned long)countByEnumeratingWithState: (struct __objcFastEnumerationState \
> *)state +                                     objects:(id *)stackbuf 
> +                                       count:(unsigned long)len;
> +@end
> +
> +@implementation MyArray : TestsuiteObject
> +- (id) initWithLength: (unsigned int)l
> +	      objects: (id *)o
> +{
> +  length = l;
> +  objects = o;
> +  mutated = 0;
> +  return self;
> +}
> +- (void) mutate
> +{
> +  mutated = 1;
> +}
> +- (unsigned long)countByEnumeratingWithState: (struct \
> __objcFastEnumerationState*)state  +		  		     objects: (id*)stackbuf
> +			 	       count: (unsigned long)len
> +{
> +  unsigned long i, batch_size;
> +
> +  /* We keep how many objects we served in the state->state counter.  So the next \
> batch +     will contain up to length - state->state objects.  */
> +  batch_size = length - state->state;
> +
> +  /* Make obvious adjustments.  */
> +  if (batch_size < 0)
> +    batch_size = 0;
> +
> +  if (batch_size > len)
> +    batch_size = len;
> +
> +  /* Copy the objects.  */
> +  for (i = 0; i < batch_size; i++)
> +    stackbuf[i] = objects[i];
> +
> +  state->state += batch_size;
> +  state->itemsPtr = stackbuf;
> +  state->mutationsPtr = &mutated;
> +
> +  return batch_size;
> +}
> +@end
> +
> +int check = 0;
> +
> +int
> +main()
> +{
> +  id *objects = malloc (sizeof (id) * 2);
> +  objects[0] = @"a";
> +  objects[1] = @"b";
> +
> +  MyArray *array = [[MyArray alloc] initWithLength: 2 objects: objects];
> +
> +  int someVar = 0;
> +  for (id object in array) {
> +    switch (someVar) {
> +      case 0:
> +	break;
> +    }
> +    ++check;
> +  }
> +
> +  if (check != 2)
> +    abort ();
> +  return 0;
> +}
> 
> 
> 	Jakub
> 


[prev in list] [next in list] [prev in thread] [next in thread]