diff mbox

Eliminate -var-create error for optzd ptr to struct

Message ID 1459888769-18875-1-git-send-email-donb@codesourcery.com
State New
Headers show

Commit Message

Don Breazeal April 5, 2016, 8:39 p.m. UTC 
On 4/5/2016 12:00 PM, Pedro Alves wrote:
> On 04/05/2016 07:50 PM, Don Breazeal wrote:
>> Hi Yao,
>>
>> On 4/5/2016 5:52 AM, Yao Qi wrote:
>>> Don Breazeal <donb@codesourcery.com> writes:
>>
>>>> +      CATCH (ex, RETURN_MASK_ERROR)
>>>> +	{
>>>> +	  /* If we get an error, assume the value is not optimized out.  */
>>>> +	  return 0;
>>>
>>> Why don't we fall back to checking value->optimized_out below?  Some
>>> bits/pieces of value are optimized out, but reading the rest of
>>> bits/piece may trigger the memory error.  In this case, the value is
>>> optimized out too.  We can do this...
>>>
>>>          TRY
>>>          {
>>>            value_fetch_lazy (value);
>>>          }
>>>          CATCH (ex, RETURN_MASK_ERROR)
>>>          {
>>>            /* Fall back to checking value->optimized_out.  */
>>>          }
>>>          END_CATCH
>>>
>>> What do you think?
>>
>> Of course, that makes more sense, thanks.
>>
>>> Note that, after this patch, value_optimized_out will no longer throw
>>> exceptions, some TRY/CATCH in value_optimized_out's callers can be
>>> removed, such as gdbscm_value_optimized_out_p and
>>> valpy_get_is_optimized_out.  This can be done in a follow-up patch.
>>
>> I've done a more thorough audit of the call sites for value_optimized_out
>> than I did previously, and the only places where an enclosing TRY/CATCH can
>> be removed are the two you name.  There is one other place where it is
>> called inside a TRY/CATCH, but there are other functions that could throw
>> errors called there as well.
>>
>> I will create a follow-up patch.
>>
>> I changed this patch as you suggest above, as well as changing
>> RETURN_MASK_ERROR to RETURN_MASK_ALL.  The TRY/CATCH blocks that are
>> going to be removed use RETURN_MASK_ALL, and I thought that this patch
>> should maintain the same level of coverage.
> 
> Please don't.  A RETURN_MASK_ALL swallows Ctrl-C/QUIT, and that's almost
> always a bug.  The cases you mention translate a QUIT to a python/scheme
> exception, which is not the same as just swallowing the exception.

Patch below changes that back.  Pedro, thanks for clarifying.

--Don

----
This patch eliminates an error thrown when accessing the value of a
pointer to a structure where the pointer has been optimized out and
'set print object' is 'on'.  The error shows up as the rather ugly
value of the pointer variable in Eclipse.

If 'set print object' is 'on', GDB tries to determine the actual
(derived) type of the object rather than the declared type, which
requires dereferencing the pointer, which in this cases throws an
error because the pointer has been optimized out.

The fix is to simply ignore the 'print object on' setting for
pointers or references to structures when they have been optimized
out.  This means we just get the declared type instead of the actual
type, because in this case that's the best that we can do.

To implement the fix, value_optimized_out was modified so that it
no longer throws an error when it fails to fetch the specified
value.  Instead, it just checks value->optimized_out.  If we can't
definitively say that the value is optimized out, then we assume
it is not.

I'm working on setting things in motion for a patch to Eclipse that
recognizes optimized-out pointer-to-struct in this scenario and
prevents any subsequent attempt to dereference it from that end.

Tested on bare-metal powerpc board with Linux x86 host and Linux
native x86_64.

gdb/ChangeLog:
2016-04-05  Don Breazeal  <donb@codesourcery.com>

	* value.c (value_actual_type): Don't try to get rtti type
	of the value if it has been optimized out.
	(value_optimized_out): If a memory access error occurs,
	just check vaue->optimized_out.


---
 gdb/value.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Yao Qi April 6, 2016, 9:04 a.m. UTC | #1 
Don Breazeal <donb@codesourcery.com> writes:

>> Please don't.  A RETURN_MASK_ALL swallows Ctrl-C/QUIT, and that's almost
>> always a bug.  The cases you mention translate a QUIT to a python/scheme
>> exception, which is not the same as just swallowing the exception.
>
> Patch below changes that back.  Pedro, thanks for clarifying.
>

No, it doesn't.

> @@ -1433,7 +1434,17 @@ value_optimized_out (struct value *value)
>    /* We can only know if a value is optimized out once we have tried to
>       fetch it.  */
>    if (VEC_empty (range_s, value->optimized_out) && value->lazy)
> -    value_fetch_lazy (value);
> +    {
> +      TRY
> +	{
> +	  value_fetch_lazy (value);
> +	}
> +      CATCH (ex, RETURN_MASK_ALL)

It should be RETURN_MASK_ERROR.

> +	{
> +	  /* Fall back to checking value->optimized_out.  */
> +	}
> +      END_CATCH
> +    }

Otherwise, patch is good to me.
Don Breazeal April 6, 2016, 9:41 p.m. UTC | #2 
On 4/6/2016 2:04 AM, Yao Qi wrote:
> Don Breazeal <donb@codesourcery.com> writes:
> 
>>> Please don't.  A RETURN_MASK_ALL swallows Ctrl-C/QUIT, and that's almost
>>> always a bug.  The cases you mention translate a QUIT to a python/scheme
>>> exception, which is not the same as just swallowing the exception.
>>
>> Patch below changes that back.  Pedro, thanks for clarifying.
>>
> 
> No, it doesn't.
> 
>> @@ -1433,7 +1434,17 @@ value_optimized_out (struct value *value)
>>    /* We can only know if a value is optimized out once we have tried to
>>       fetch it.  */
>>    if (VEC_empty (range_s, value->optimized_out) && value->lazy)
>> -    value_fetch_lazy (value);
>> +    {
>> +      TRY
>> +	{
>> +	  value_fetch_lazy (value);
>> +	}
>> +      CATCH (ex, RETURN_MASK_ALL)
> 
> It should be RETURN_MASK_ERROR.

Sorry about that.
/me shakes head at self for goofing up the easy stuff.

> 
>> +	{
>> +	  /* Fall back to checking value->optimized_out.  */
>> +	}
>> +      END_CATCH
>> +    }
> 
> Otherwise, patch is good to me.
> 
Thanks Yao.  This is now corrected and pushed in.

Question: in light of Pedro's comments regarding
gdbscm_value_optimized_out_p and valpy_get_is_optimized_out:

> A RETURN_MASK_ALL swallows Ctrl-C/QUIT, and that's almost
> always a bug.  The cases you mention translate a QUIT to
> a python/scheme exception, which is not the same as just
> swallowing the exception.

Would you still like for me to follow up with a patch to remove the
TRY/CATCH blocks around the calls to value_optimized_out in those two
functions?  Or do we want to leave it as-is so that the QUIT handling
remains unchanged?

Thanks
--Don
Pedro Alves April 6, 2016, 10:24 p.m. UTC | #3 
On 04/06/2016 10:41 PM, Don Breazeal wrote:
> On 4/6/2016 2:04 AM, Yao Qi wrote:

> Question: in light of Pedro's comments regarding
> gdbscm_value_optimized_out_p and valpy_get_is_optimized_out:
> 
>> A RETURN_MASK_ALL swallows Ctrl-C/QUIT, and that's almost
>> always a bug.  The cases you mention translate a QUIT to
>> a python/scheme exception, which is not the same as just
>> swallowing the exception.
> 
> Would you still like for me to follow up with a patch to remove the
> TRY/CATCH blocks around the calls to value_optimized_out in those two
> functions?  Or do we want to leave it as-is so that the QUIT handling
> remains unchanged?

Please leave them as is.  We need to translate gdb exceptions to Python/Scheme
exceptions at the gdb <-> extension-language boundaries.  Those gdb
functions in question will be returning to the Python/Scheme runtimes, which
are not prepared to unwind correctly on a gdb exception.

Thanks,
Pedro Alves
Yao Qi April 7, 2016, 2:11 p.m. UTC | #4 
On 06/04/16 22:41, Don Breazeal wrote:
> Would you still like for me to follow up with a patch to remove the
> TRY/CATCH blocks around the calls to value_optimized_out in those two
> functions?  Or do we want to leave it as-is so that the QUIT handling
> remains unchanged?

No, I was wrong on that point.  Please leave it as-is.
diff mbox

Patch

diff --git a/gdb/value.c b/gdb/value.c
index 8268b08..8a210e7 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1205,7 +1205,8 @@  value_actual_type (struct value *value, int resolve_simple_types,
       if ((TYPE_CODE (result) == TYPE_CODE_PTR
 	   || TYPE_CODE (result) == TYPE_CODE_REF)
 	  && TYPE_CODE (check_typedef (TYPE_TARGET_TYPE (result)))
-	     == TYPE_CODE_STRUCT)
+	     == TYPE_CODE_STRUCT
+	  && !value_optimized_out (value))
         {
           struct type *real_type;
 
@@ -1433,7 +1434,17 @@  value_optimized_out (struct value *value)
   /* We can only know if a value is optimized out once we have tried to
      fetch it.  */
   if (VEC_empty (range_s, value->optimized_out) && value->lazy)
-    value_fetch_lazy (value);
+    {
+      TRY
+	{
+	  value_fetch_lazy (value);
+	}
+      CATCH (ex, RETURN_MASK_ALL)
+	{
+	  /* Fall back to checking value->optimized_out.  */
+	}
+      END_CATCH
+    }
 
   return !VEC_empty (range_s, value->optimized_out);
 }