| Message ID | 1475771231-1739-1-git-send-email-lgustavo@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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?
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".
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).
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
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
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
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 >
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.
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"