diff mbox

Query about the TREE_TYPE field

Message ID 546B7414.3070500@redhat.com
State New
Headers show

Commit Message

Andrew MacLeod Nov. 18, 2014, 4:30 p.m. UTC 
On 11/18/2014 09:52 AM, Andrew MacLeod wrote:
> On 11/18/2014 09:40 AM, Jason Merrill wrote:
>> On 11/18/2014 09:26 AM, Andrew MacLeod wrote:
>>> I was poking around attribs.c while trial running my tree-type-safety
>>> stuff, and it triggered something in decl_attributes() that seems fishy
>>> to me.  It looks like it was part of the fix for
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=35315
>>>
>>> decl_attributes() can be passed a tree node which is either decl or a
>>> type, but we get to this little snippet:
>>>
>>>        if (spec->type_required && DECL_P (*anode))
>>>          {
>>>            anode = &TREE_TYPE (*anode);
>>>            /* Allow ATTR_FLAG_TYPE_IN_PLACE for the type's naming 
>>> decl.  */
>>>            if (!(TREE_CODE (*anode) == TYPE_DECL
>>>                  && *anode == TYPE_NAME (TYPE_MAIN_VARIANT
>>>                                          (TREE_TYPE (*anode)))))
>>>              flags &= ~(int) ATTR_FLAG_TYPE_IN_PLACE;
>>>          }
>>>
>>> anode is changed to point to the TREE_TYPE of the original decl, and
>>> *then* checks if it is a TYPE_DECL...   That doesnt seem right to me..
>>> we can't have a TYPE_DECL as a TREE_TYPE can we?
>>
>> No.
>>
>>> is that code suppose to be checking is the original DECL is a TYPE_DECL
>>> rather than the TREE_TYPE?
>>
>> I think so.
>>
>>> Maybe the assignment to anode should be after the if instead of in 
>>> front
>>> of it?
>>
>> Probably.
>>
>> Strange that the 35315 patch fixed the testcase with that change 
>> being a placebo...
>>
>> Jason
>>
> Indeed.   The condition is negated,  so effectively  it becomes an 
> always true condition and the ATTR_FLAG_TYPE_IN_PLACE is always turned 
> off when a DECL is passed and a type is required..
>
> maybe that works most/all of the time?   huh, that also means the code 
> is the same as it was before the patch :-P
>
> maybe one of the follow up patches in bugzilla supercede it?
>
> Andrew
>
I tried doing the if before chaning to TREE_TYPE...  absolutely no 
effect on the testsuite or anything else :-)

What do you think, should I check this in?   What is there is clearly 
incorrect.    we could also revert the original patch since that is what 
the code base is effectively is doing at the moment...

Andrew

Comments

Jeff Law Nov. 18, 2014, 6:36 p.m. UTC | #1 
On 11/18/14 09:30, Andrew MacLeod wrote:

> I tried doing the if before chaning to TREE_TYPE...  absolutely no
> effect on the testsuite or anything else :-)
>
> What do you think, should I check this in?   What is there is clearly
> incorrect.    we could also revert the original patch since that is what
> the code base is effectively is doing at the moment...
What I tend to do in these situations is roll back to the version prior 
to the "fix" and try the testcase with that compiler.  Then I can walk 
through under GDB control to find out whatever I need.

Clearly something needs to change here, but the question is what :-) 
And I think rolling back and debugging that compiler is the best way to 
know get more information to allow this to move forward.

Jeff
diff mbox

Patch

	* attribs.c (decl_attributes): Check for TYPE_DECL before switching to
	using the TREE_TYPE.

Index: attribs.c
===================================================================
*** attribs.c	(revision 217234)
--- attribs.c	(working copy)
*************** decl_attributes (tree *node, tree attrib
*** 501,512 ****
  	 the decl's type in place here.  */
        if (spec->type_required && DECL_P (*anode))
  	{
- 	  anode = &TREE_TYPE (*anode);
  	  /* Allow ATTR_FLAG_TYPE_IN_PLACE for the type's naming decl.  */
  	  if (!(TREE_CODE (*anode) == TYPE_DECL
  		&& *anode == TYPE_NAME (TYPE_MAIN_VARIANT
  					(TREE_TYPE (*anode)))))
  	    flags &= ~(int) ATTR_FLAG_TYPE_IN_PLACE;
  	}
  
        if (spec->function_type_required && TREE_CODE (*anode) != FUNCTION_TYPE
--- 501,512 ----
  	 the decl's type in place here.  */
        if (spec->type_required && DECL_P (*anode))
  	{
  	  /* Allow ATTR_FLAG_TYPE_IN_PLACE for the type's naming decl.  */
  	  if (!(TREE_CODE (*anode) == TYPE_DECL
  		&& *anode == TYPE_NAME (TYPE_MAIN_VARIANT
  					(TREE_TYPE (*anode)))))
  	    flags &= ~(int) ATTR_FLAG_TYPE_IN_PLACE;
+ 	  anode = &TREE_TYPE (*anode);
  	}
  
        if (spec->function_type_required && TREE_CODE (*anode) != FUNCTION_TYPE