[PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Luis Machado]

I noticed that testing aarch64-elf gdb with a physical board
ran into issues with gdb.python/py-value.exp. Further investigation showed
that we were actually trying to dereference a NULL pointer (argv) when trying
to access argv[0].

Being bare-metal, argv is not guaranteed to be there. So we need to make sure
argv is sane before accessing argv[0].

After fixing that, i noticed we were assuming a value of 1 for argc, which is
also not true, as i see 0 in my tests.

The following patch fixes up the test program to check for a NULL argv and also
touches the testcase itself to expect either 0 or 1 for argc depending on the
presence of argv, which is something we check early in the test.

This gives me full passes for aarch64-elf when running gdb.python/py-value.exp
and doesn't regress things on x86-64.

Ok?

gdb/testsuite/ChangeLog:

2016-10-06  Luis Machado  <lgustavo@codesourcery.com>

	* gdb.python/py-value.c (main): Check if argv is NULL before using it.
	* gdb.python/py-value.exp (test_value_in_inferior): Adjust initial
	argc value based on the existence of argv.
---
 gdb/testsuite/ChangeLog               |  6 ++++++
 gdb/testsuite/gdb.python/py-value.c   |  6 +++++-
 gdb/testsuite/gdb.python/py-value.exp | 13 +++++++++++--
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index d4ae9df..e1d689a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2016-10-06  Luis Machado  <lgustavo@codesourcery.com>
+
+	* gdb.python/py-value.c (main): Check if argv is NULL before using it.
+	* gdb.python/py-value.exp (test_value_in_inferior): Adjust initial
+	argc value based on the existence of argv.
+
 2016-10-03  Antoine Tremblay  <antoine.tremblay@ericsson.com>
 2016-10-03  Simon Marchi  <simon.marchi@ericsson.com>
 
diff --git a/gdb/testsuite/gdb.python/py-value.c b/gdb/testsuite/gdb.python/py-value.c
index 586a0fd..8408e1e 100644
--- a/gdb/testsuite/gdb.python/py-value.c
+++ b/gdb/testsuite/gdb.python/py-value.c
@@ -82,7 +82,7 @@ char **save_argv;
 int
 main (int argc, char *argv[])
 {
-  char *cp = argv[0]; /* Prevent gcc from optimizing argv[] out.  */
+  char *cp;
   struct s s;
   union u u;
   PTR x = &s;
@@ -99,6 +99,10 @@ main (int argc, char *argv[])
   const char *sn = 0;
   struct str *xstr;
 
+  /* Prevent gcc from optimizing argv[] out.  */
+  if (argv != NULL)
+    cp = argv[0];
+
   s.a = 3;
   s.b = 5;
   u.a = 7;
diff --git a/gdb/testsuite/gdb.python/py-value.exp b/gdb/testsuite/gdb.python/py-value.exp
index 57a9ba1..bb802bf 100644
--- a/gdb/testsuite/gdb.python/py-value.exp
+++ b/gdb/testsuite/gdb.python/py-value.exp
@@ -274,10 +274,19 @@ proc test_value_in_inferior {} {
   gdb_test "python argc_notlazy.fetch_lazy()"
   gdb_test "python print (argc_lazy.is_lazy)" "True"
   gdb_test "python print (argc_notlazy.is_lazy)" "False"
-  gdb_test "print argc" " = 1" "sanity check argc"
+
+  # If argv is not available, then argc is likely not available as well, thus
+  # we expect it to be 0.
+  set argc_value 0
+  if { $has_argv0 } {
+    set argc_value 1
+  }
+
+  gdb_test "print argc" " = $argc_value" "sanity check argc"
+
   gdb_test "python print (argc_lazy.is_lazy)" "\r\nTrue"
   gdb_test_no_output "set argc=2"
-  gdb_test "python print (argc_notlazy)" "\r\n1"
+  gdb_test "python print (argc_notlazy)" "\r\n$argc_value"
   gdb_test "python print (argc_lazy)" "\r\n2"
   gdb_test "python print (argc_lazy.is_lazy)" "False"
 
-- 
2.7.4

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Yao Qi]

On Thu, Oct 6, 2016 at 5:27 PM, Luis Machado <lgustavo@codesourcery.com> wrote:
> I noticed that testing aarch64-elf gdb with a physical board
> ran into issues with gdb.python/py-value.exp. Further investigation showed
> that we were actually trying to dereference a NULL pointer (argv) when trying
> to access argv[0].
>
> Being bare-metal, argv is not guaranteed to be there. So we need to make sure
> argv is sane before accessing argv[0].
>
> After fixing that, i noticed we were assuming a value of 1 for argc, which is
> also not true, as i see 0 in my tests.

If I understand C standard
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf correctly,
"argv" can't be NULL.

5.1.2.2.1 Program startup
...
 The value of argc shall be nonnegative.
 argv[argc] shall be a null pointer.

The first one implies that argc can be zero, and the second one implies
argv can't be NULL.  In this case, argc is zero, so argv[0] can be
dereferenced.

> @@ -99,6 +99,10 @@ main (int argc, char *argv[])
>    const char *sn = 0;
>    struct str *xstr;
>
> +  /* Prevent gcc from optimizing argv[] out.  */
> +  if (argv != NULL)
> +    cp = argv[0];
> +

As I mentioned above, argv shouldn't be NULL.  Is it your C runtime
problem?

>    s.a = 3;
>    s.b = 5;
>    u.a = 7;
> diff --git a/gdb/testsuite/gdb.python/py-value.exp b/gdb/testsuite/gdb.python/py-value.exp
> index 57a9ba1..bb802bf 100644
> --- a/gdb/testsuite/gdb.python/py-value.exp
> +++ b/gdb/testsuite/gdb.python/py-value.exp
> @@ -274,10 +274,19 @@ proc test_value_in_inferior {} {
>    gdb_test "python argc_notlazy.fetch_lazy()"
>    gdb_test "python print (argc_lazy.is_lazy)" "True"
>    gdb_test "python print (argc_notlazy.is_lazy)" "False"
> -  gdb_test "print argc" " = 1" "sanity check argc"
> +
> +  # If argv is not available, then argc is likely not available as well, thus
> +  # we expect it to be 0.

The comment isn't accurate.  Can you change it to mention "argc can
be nonnegative."?

> +  set argc_value 0
> +  if { $has_argv0 } {
> +    set argc_value 1
> +  }
> +
> +  gdb_test "print argc" " = $argc_value" "sanity check argc"
> +

Instead of setting argc_value manually, we can use get_integer_valueof
to get value of "argc", and save it in argc_value.  The purpose of
tests here is that after changing argc, argc_notlazy still equals to the
old argc while argc_lazy equals to the changed argc.  It doesn't matter
the old argc is 1 or 0.

>    gdb_test "python print (argc_lazy.is_lazy)" "\r\nTrue"
>    gdb_test_no_output "set argc=2"
> -  gdb_test "python print (argc_notlazy)" "\r\n1"
> +  gdb_test "python print (argc_notlazy)" "\r\n$argc_value"
>    gdb_test "python print (argc_lazy)" "\r\n2"
>    gdb_test "python print (argc_lazy.is_lazy)" "False"
>

Changes in gdb.python/py-value.exp are reasonable to me, but need
some minor changes as I suggested above.  Changes in
gdb.python/py-value.c are not, and you need to fix your C runtime, IMO.

-- 
Yao (齐尧)

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Luis Machado]

On 10/07/2016 05:23 AM, Yao Qi wrote:
> On Thu, Oct 6, 2016 at 5:27 PM, Luis Machado <lgustavo@codesourcery.com> wrote:
>> I noticed that testing aarch64-elf gdb with a physical board
>> ran into issues with gdb.python/py-value.exp. Further investigation showed
>> that we were actually trying to dereference a NULL pointer (argv) when trying
>> to access argv[0].
>>
>> Being bare-metal, argv is not guaranteed to be there. So we need to make sure
>> argv is sane before accessing argv[0].
>>
>> After fixing that, i noticed we were assuming a value of 1 for argc, which is
>> also not true, as i see 0 in my tests.
>
> If I understand C standard
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf correctly,
> "argv" can't be NULL.
>
> 5.1.2.2.1 Program startup
> ...
>  The value of argc shall be nonnegative.
>  argv[argc] shall be a null pointer.
>
> The first one implies that argc can be zero, and the second one implies
> argv can't be NULL.  In this case, argc is zero, so argv[0] can be
> dereferenced.
>

Ah, that makes sense indeed. Let me check the startup code and i'll get 
back to this patch (that may not be needed then).

Thanks,
Luis

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Luis Machado]

On 10/07/2016 05:23 AM, Yao Qi wrote:
> On Thu, Oct 6, 2016 at 5:27 PM, Luis Machado <lgustavo@codesourcery.com> wrote:
>> I noticed that testing aarch64-elf gdb with a physical board
>> ran into issues with gdb.python/py-value.exp. Further investigation showed
>> that we were actually trying to dereference a NULL pointer (argv) when trying
>> to access argv[0].
>>
>> Being bare-metal, argv is not guaranteed to be there. So we need to make sure
>> argv is sane before accessing argv[0].
>>
>> After fixing that, i noticed we were assuming a value of 1 for argc, which is
>> also not true, as i see 0 in my tests.
>
> If I understand C standard
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf correctly,
> "argv" can't be NULL.
>
> 5.1.2.2.1 Program startup
> ...
>  The value of argc shall be nonnegative.
>  argv[argc] shall be a null pointer.
>
> The first one implies that argc can be zero, and the second one implies
> argv can't be NULL.  In this case, argc is zero, so argv[0] can be
> dereferenced.
>

I went back and read the standard and we're dealing with a freestanding 
environment for bare metal here.

The descriptions above seem to make sense for a hosted environment, but 
not for a freestanding one, correct?

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Yao Qi]

On Fri, Oct 7, 2016 at 1:08 PM, Luis Machado <lgustavo@codesourcery.com> wrote:
>
> I went back and read the standard and we're dealing with a freestanding
> environment for bare metal here.
>
> The descriptions above seem to make sense for a hosted environment, but not
> for a freestanding one, correct?
>

IMO, bare metal != freestadning environment.  Since "main" function is used,
it is a hosted environment.  See
https://gcc.gnu.org/onlinedocs/gcc/Standards.html

"a hosted environment, which is not required, in which all the library
facilities are provided and startup is through a function int main
(void) or int main (int, char *[])."

On the other hand, in the C standard, function "main" is only mentioned in
the section of "5.1.2.2 Hosted environment".

-- 
Yao (齐尧)

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Luis Machado]

On 10/07/2016 09:48 AM, Yao Qi wrote:
> On Fri, Oct 7, 2016 at 1:08 PM, Luis Machado <lgustavo@codesourcery.com> wrote:
>>
>> I went back and read the standard and we're dealing with a freestanding
>> environment for bare metal here.
>>
>> The descriptions above seem to make sense for a hosted environment, but not
>> for a freestanding one, correct?
>>
>
> IMO, bare metal != freestadning environment.  Since "main" function is used,
> it is a hosted environment.  See
> https://gcc.gnu.org/onlinedocs/gcc/Standards.html

I'm slightly confused. Are you implying that using main makes this a 
hosted environment even though there is no underlying OS facility at play?

The documentation states freestanding environments can use any type of 
startup routine they want, only having to honor a small subset of clauses.

This seems to imply that the use of "main" is allowed and the lack of 
underlying OS facilities make it freestanding.

Skipping the discussion of whether we have a hosted x freestanding 
environment, the testcase itself uses main/argc/argv. Does that mean it 
is supposed to be exercised only on hosted environments or only on 
targets providing sane argc/argv values?

>
> "a hosted environment, which is not required, in which all the library
> facilities are provided and startup is through a function int main
> (void) or int main (int, char *[])."
>
> On the other hand, in the C standard, function "main" is only mentioned in
> the section of "5.1.2.2 Hosted environment".
>

They are mentioned there because it is a requirement for a hosted 
environment but, as mentioned above, freestanding programs could have a 
function called main as the startup function. There is no restriction on 
that part, correct?

More importantly, does it really matter, from a gdb testsuite point of 
view, whether the freestanding implementation provides main/argc/argv or 
not? The tests get exercised anyway, with only a small adjustment being 
required to make it not crash due to an assumption that is not always 
true (presence of argc/argv).

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Yao Qi]

Well, I am not a C standard guru, so I can be wrong.  I post
this question to gcc-help
https://gcc.gnu.org/ml/gcc-help/2016-10/msg00031.html

-- 
Yao (齐尧)

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Luis Machado]

On 10/07/2016 11:51 AM, Yao Qi wrote:
> Well, I am not a C standard guru, so I can be wrong.  I post
> this question to gcc-help
> https://gcc.gnu.org/ml/gcc-help/2016-10/msg00031.html
>

That is ok. Let's see what comes out of that.

Thanks,
Luis

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Luis Machado]

Hi Yao,

Given Martin's reply, do you think this is now acceptable with possibly 
some minor fixups?

Luis
On 10/07/2016 11:59 AM, Luis Machado wrote:
> On 10/07/2016 11:51 AM, Yao Qi wrote:
>> Well, I am not a C standard guru, so I can be wrong.  I post
>> this question to gcc-help
>> https://gcc.gnu.org/ml/gcc-help/2016-10/msg00031.html
>>
>
> That is ok. Let's see what comes out of that.
>
> Thanks,
> Luis
>

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Yao Qi]

On Mon, Oct 10, 2016 at 6:04 PM, Luis Machado <lgustavo@codesourcery.com> wrote:
> Hi Yao,
>
> Given Martin's reply, do you think this is now acceptable with possibly some
> minor fixups?
>

Yes.  My comments to changes in gdb.python/py-value.exp still stand.

-- 
Yao (齐尧)

Re: [PATCH] Fixup gdb.python/py-value.exp for bare-metal aarch64-elf

[Kevin Buettner]

On Thu, 6 Oct 2016 11:27:11 -0500
Luis Machado <lgustavo@codesourcery.com> wrote:

> I noticed that testing aarch64-elf gdb with a physical board
> ran into issues with gdb.python/py-value.exp. Further investigation showed
> that we were actually trying to dereference a NULL pointer (argv) when trying
> to access argv[0].
> 
> Being bare-metal, argv is not guaranteed to be there. So we need to make sure
> argv is sane before accessing argv[0].
> 
> After fixing that, i noticed we were assuming a value of 1 for argc, which is
> also not true, as i see 0 in my tests.
> 
> The following patch fixes up the test program to check for a NULL argv and also
> touches the testcase itself to expect either 0 or 1 for argc depending on the
> presence of argv, which is something we check early in the test.
> 
> This gives me full passes for aarch64-elf when running gdb.python/py-value.exp
> and doesn't regress things on x86-64.
> 
> Ok?

FWIW, I've run into some of these same problems while testing against
some of the simulators.  In such an environment, I think it makes
sense to pass in NULL for argv.

I'd like to see your patch (or something like it) go in.  I think it's easier to
adjust the tests than it is to adjust the startup code in a bunch of different
places.

Kevin