=?UTF-8?Q?ObjC=20patch:=20fix=20CLASS=5FHAS=5FEXCEPTION=5FATTR?=

This patch fixes one of the pending FIXMEs with the big ObjC patch committed
a few days ago, which is

#define CLASS_HAS_EXCEPTION_ATTR(CLASS) ((CLASS)->type.lang_flag_0)

The problem there is that that flag is already in use by C and C++ and there
is potential for conflict if it is used for arbitrary tree nodes (as it is by
the new code).

But we don't need to use it on arbitrary tree nodes.  To implement what is
needed, we only need to use it for CLASS_INTERFACE_TYPE nodes, which are
Objective-C-specific frontend nodes, and no conflict can arise. :-)

This patch removes copying the CLASS_HAS_EXCEPTION_ATTR() flag from the
CLASS_INTERFACE_TYPE node to the corresponding struct when creating the
struct, as there is no need to have that flag there - it's never used.
This removes any possibility of conflict.  I think that copying of the flag
into the struct was inspired by the copying of the deprecated flag into the
struct, but in this case there is no reason to copy the flag.

I also added a basic testcase for "attribute((objc_exception))".  It only checks that
the attribute is accepted correctly; it would be good to have a testcase checking metadata
generation for Apple 64-bit (I did check that the flag gets detected correctly during
metadata generation by just hacking some code into the compiler; it would be nice to have a
complete standard test for the Apple 64-bit that checks the actual output).

Ok to commit ?

Thanks

PS: The other missing changes

On Feb 22, 2011, at 3:22 PM, Nicola Pero wrote:
> This patch fixes one of the pending FIXMEs with the big ObjC patch committed
> a few days ago, which is

> I also added a basic testcase for "attribute((objc_exception))".  It only checks that
> the attribute is accepted correctly; it would be good to have a testcase checking metadata
> generation for Apple 64-bit (I did check that the flag gets detected correctly during
> metadata generation by just hacking some code into the compiler; it would be nice to have a
> complete standard test for the Apple 64-bit that checks the actual output).

You can test the metadata by using scan and just having a snippet that you know is correct.

An example from the testsuite:

	/* { dg-final { scan-assembler ".globl _a.*.data.*.space\[\t \]1" } } */

. matches \n as well, and the thing to match is the entire .s file produced.  If you want to control it, you'd want [^\n] for all but newline.  You'd want to start from the symbol (something unique) and then just match the output you expect.  If the label is uncontrolled (L435), don't do that, but if it is something like L_METADATA_CLASS_T, then it should be safe until we alter the name and have to update the testcase.

> Ok to commit ?

Ok.  I'll pre-approve adding scan to the testcase, if you want, you don't have to.

Hi Nicola,

On 23 Feb 2011, at 01:52, Mike Stump wrote:

> On Feb 22, 2011, at 3:22 PM, Nicola Pero wrote:
>> This patch fixes one of the pending FIXMEs with the big ObjC patch  
>> committed
>> a few days ago, which is

thanks for looking at this - a solution that is local to ObjC is much  
better.

>> I also added a basic testcase for "attribute((objc_exception))".   
>> It only checks that
>> the attribute is accepted correctly; it would be good to have a  
>> testcase checking metadata
>> generation for Apple 64-bit (I did check that the flag gets  
>> detected correctly during
>> metadata generation by just hacking some code into the compiler; it  
>> would be nice to have a
>> complete standard test for the Apple 64-bit that checks the actual  
>> output).
>
> You can test the metadata by using scan and just having a snippet  
> that you know is correct.
>
> An example from the testsuite:
>
> 	/* { dg-final { scan-assembler ".globl _a.*.data.*.space\[\t  
> \]1" } } */
>
> . matches \n as well, and the thing to match is the entire .s file  
> produced.  If you want to control it, you'd want [^\n] for all but  
> newline.  You'd want to start from the symbol (something unique) and  
> then just match the output you expect.  If the label is uncontrolled  
> (L435), don't do that, but if it is something like  
> L_METADATA_CLASS_T, then it should be safe until we alter the name  
> and have to update the testcase.
>
>> Ok to commit ?
>
> Ok.  I'll pre-approve adding scan to the testcase, if you want, you  
> don't have to.

when there is next some time, I was planning to:

(a) move all the meta-data tests to the torture section (since they  
verify code-gen)
(b) this seems a weak area of testing for the GNU runtime, so add to/ 
strengthen testing there.
(c) perhaps segregate these particular tests by runtime (these should  
be the only tests that require that).

whilst it is possible to cater for (c) by putting target conditionals  
on the scan-assembler lines, they do start to get unwieldy (see the  
few examples dealing with NeXT m32/m64/ppc/i386) - and I don't believe  
that DejaGnu will allow us to break lines.  In this instance - it  
might be clearer to find a different solution for dealing with  
multiple runtimes.

cheers
Iain

> when there is next some time, I was planning to:
> 
> (a) move all the meta-data tests to the torture section (since they  
> verify code-gen)
> (b) this seems a weak area of testing for the GNU runtime, so add to/ 
> strengthen testing there.
> (c) perhaps segregate these particular tests by runtime (these should  
> be the only tests that require that).

Sounds good.  A few comments:

 (a) I agree that we want to run the meta-data tests with the maximum amount
of optimizations to be sure that nothing is optimized away in the metadata
structures; I wonder if we need to test the full list of intermediate optimization
levels [full torture tests take *ages* to run ;-)].  Ideally, everything should
be run under torture (after all, the more you test the better); in practice the
testsuite would take so much longer if we did, and I'm not sure it'd help that
much.  Maybe for meta-data tests we can limit ourselves to a run with no optimizations,
and a run with the maximum amount of optimizations ?  Is that a reasonable
compromise ? :-)

 (b) the GNU runtime tests metadata generation mostly with execution tests.
We now have tests for execution of all of the functions in the "modern" GNU runtime API.
These implicitly test metadata generation: eg, getting the list of protocols from
the runtime won't work unless the compiler has put that list there in the right
format that the runtime can read and work on.

 (c) testing meta-data generation as in (b) is quite a good way to test it across
runtimes.  The API now being almost identical, we can use the same test on all runtimes.
The smaller API differences (eg, with older versions of Darwin) can be managed with #ifdefs.

 (d) of course, writing tests for meta-data generation as in (b) is harder/less immediate,
because you have to figure out how an error in the meta-data generation would affect "end users"
and then write a piece of ObjC code that wouldn't work due to that bug, and that is your
testcase.  Eg, in the case of attribute((objc_exception)), this seems to set a RO_EXCEPTION
flag for the class in the Apple runtime.  Someone would have to figure out how/when that
flag is used to write a testcase that fails when the flag is incorrectly set.  It's obviously
much easier to just write a test that uses scan-assembler to look for the flag in the compiler
output.  (on the other hand, figuring out when that flag is used, and writing a testcase for it,
would mean we can then try that testcase with the GNU runtime and see if we have a bug/missing
feature/compatibility problem in there, so it may still be worth it!). ;-)

Summarizing, I don't mind scan-assembler meta-data generation tests being added (great!), but
I personally would rather write ObjC execution tests that can be more trivially used with different
runtimes and platforms :-)

Thanks

On Feb 24, 2011, at 2:50 AM, Nicola Pero wrote:
> Sounds good.  A few comments:
> 
> I wonder if we need to test the full list of intermediate optimization
> levels

Actually, I question the need to torture them.  The optimizer doesn't optimize metadata, so there is no need to test it.  The most it would do it appear or disappear data, but wether it does that or not, is best handled by directly testing the feature you use to make the data come out, once.  So, if you used TREE_USED, you test the optimizer with tree used.  If you use external reference, you test that.  If you use internal reference to an eternal routine, you test that.  And even this test, if it is already tested anywhere in any of the testsuites already, doesn't really need to be tested by us.