From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6045 invoked by alias); 9 Jul 2018 13:16:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 6023 invoked by uid 89); 9 Jul 2018 13:16:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=cofounder, co-founder, UD:linkedin.com, LinkedIn X-HELO: a2-138.smtp-out.eu-west-1.amazonses.com Received: from a2-138.smtp-out.eu-west-1.amazonses.com (HELO a2-138.smtp-out.eu-west-1.amazonses.com) (54.240.2.138) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 09 Jul 2018 13:16:47 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=jl7vyxitgsfircfdhxflkj2c3tgxidze; d=farjump.io; t=1531142204; h=Subject:From:To:Date:Mime-Version:Content-Type:Content-Transfer-Encoding:In-Reply-To:References:Message-Id; bh=ONXVCg3M7DFSZFNjG7I8g7B3CCeCSIx/1aWUFvnOF+Q=; b=Neh/V8o9ZWjVUW+rwozXa9NJb3DN9bPE6kniu/j4P5bC+EXrTZkkk1vSNRxf6NYu XcDePagyIWV7RlI3y63W5ItOLrWHBtYMLvfKHp09tJzTD357cVcUVpS75ysukTP5X9o DgYjN6IJVGRrmEo1F1tk/a9012ctEjVe0TCZnC5s= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=uku4taia5b5tsbglxyj6zym32efj7xqv; d=amazonses.com; t=1531142204; h=Subject:From:To:Date:Mime-Version:Content-Type:Content-Transfer-Encoding:In-Reply-To:References:Message-Id:Feedback-ID; bh=ONXVCg3M7DFSZFNjG7I8g7B3CCeCSIx/1aWUFvnOF+Q=; b=uc2JtJmhpdE6z4aOrfqZO0weiVuUy7p3qI2OQihJCY9xaFzmAUmJXhoPaTigvDlW PK/F174Wt8b4LkjVDS3uFNJ3D4mtdTXtggt5+2NBUUkY1N2Np4NRHlDpPNvz4JMhiEs ReRy48Kv3Q1sE+kUCjZgqgzprUoTGpQoK+3tXl8o= Subject: Re: [PATCH v4] Allow using special files with File I/O functions From: =?UTF-8?Q?Julio_Guerra?= To: =?UTF-8?Q?Pedro_Alves?= , =?UTF-8?Q?gdb-patches=40s?= =?UTF-8?Q?ourceware=2Eorg?= Date: Mon, 09 Jul 2018 13:16:00 -0000 Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable In-Reply-To: <50cbe319-5b79-24dd-c615-25209d6dd390@redhat.com> References: <20180705091618.33743-1-julio@farjump.io> <0102016469ba9beb-e8338b53-74bd-46ea-91c7-eea909052532-000000@eu-west-1.amazonses.com> <50cbe319-5b79-24dd-c615-25209d6dd390@redhat.com> Message-ID: <010201647f2fec5a-727b623a-1c7f-4d7e-a9e2-e8076d9e3cc2-000000@eu-west-1.amazonses.com> X-SW-Source: 2018-07/txt/msg00213.txt.bz2 >> Remove the restriction in remote_fileio_func_open() to regular files onl= y and=0D >> add support for special file types in the File IO stat structure.=0D >> The link file type is not part of the new definitions as stat() and fsta= t()=0D >> called by remote_fileio_func_stat() and remote_fileio_func_fstat() follo= w links,=0D >> it is thus not possible to obtain this file type in the File IO stat str= ucture.=0D >>=0D >> Add tests to cover as much cases as possible, limited by the fact that s= ome=0D >> types such as FIFOs or character devices cannot be created on non-unix o= perating=0D >> systems. This limits the test cases to a regular file, a directory and t= he=0D >> standard output/input/error descriptors.=0D >>=0D >> The major goal is to be able to write advanced embedded testing function= s, like:=0D >> - using a FIFO between the embedded program and the host, instead of bei= ng=0D >> restricted to the GDB console only.=0D >> - mocking features based on host's by using some host device.=0D >>=0D >> 2018-07-05 Julio Guerra =0D >>=0D >> * remote-fileio.c (remote_fileio_func_open, remote_fileio_func_stat):=0D >> Allow using File I/O functions open(), stat() and fstat() on special=0D >> files.=0D >> * common/fileio.c (fileio_to_host_mode, fileio_mode_pack): Add new=0D >> special file types in fst_mode's definition.=0D >> (host_to_fileio_stat): Define fst_dev using the new macro definitions=0D >> and according to the file's type.=0D >> * testsuite/gdb.base/fileio.c: Add test cases to cover some special=0D >> files and file descriptors.=0D >> * testsuite/gdb.base/fileio.exp: Likewise.=0D >> * doc/gdb.texinfo: Document the changes.=0D >> * NEWS: Briefly describe the changes of File I/O operations open, stat,= =0D >> fstat.=0D > Note that testsuite and docs have their own ChangeLogs. Please see:=0D >=0D > https://sourceware.org/gdb/wiki/ContributionChecklist#ChangeLog=0D =0D Ok.=0D =0D >=0D >> Signed-off-by: Julio Guerra =0D > I'm not sure whether I asked this before, but, just in case,=0D > do you have a copyright assignment on file with the FSF?=0D > I looked for one now and couldn't find it.=0D >=0D =0D No, I don't.=0D =0D >> ---=0D >> gdb/ChangeLog | 16 ++++++=0D >> gdb/NEWS | 4 ++=0D >> gdb/common/fileio.c | 55 +++++++++++++++----=0D >> gdb/doc/gdb.texinfo | 7 ++-=0D >> gdb/remote-fileio.c | 10 +---=0D >> gdb/testsuite/gdb.base/fileio.c | 87 +++++++++++++++++++++++++++----= =0D >> gdb/testsuite/gdb.base/fileio.exp | 26 ++++++++-=0D >> include/ChangeLog | 5 ++=0D >> include/gdb/fileio.h | 19 +++++--=0D >> 9 files changed, 197 insertions(+), 32 deletions(-)=0D >>=0D >> diff --git a/gdb/ChangeLog b/gdb/ChangeLog=0D >> index 7bd0cc4f95..b937f029e3 100644=0D >> --- a/gdb/ChangeLog=0D >> +++ b/gdb/ChangeLog=0D >> @@ -1,3 +1,19 @@=0D >> +2018-07-05 Julio Guerra =0D >> +=0D >> + * remote-fileio.c (remote_fileio_func_open, remote_fileio_func_stat):= =0D >> + Allow using File I/O functions open(), stat() and fstat() on special=0D >> + files.=0D >> + * common/fileio.c (fileio_to_host_mode, fileio_mode_pack): Add new=0D >> + special file types in fst_mode's definition.=0D >> + (host_to_fileio_stat): Define fst_dev using the new macro definitions= =0D >> + and according to the file's type.=0D >> + * testsuite/gdb.base/fileio.c: Add test cases to cover some special=0D >> + files and file descriptors.=0D >> + * testsuite/gdb.base/fileio.exp: Likewise.=0D >> + * doc/gdb.texinfo: Document the changes.=0D >> + * NEWS: Briefly describe the changes of File I/O operations open, stat= ,=0D >> + fstat.=0D >> +=0D >> 2018-07-04 Tom Tromey =0D >> =0D >> * darwin-nat.c (darwin_attach_pid): Use exit_inferior.=0D >> diff --git a/gdb/NEWS b/gdb/NEWS=0D >> index 2d1d161233..4a0ab4ac52 100644=0D >> --- a/gdb/NEWS=0D >> +++ b/gdb/NEWS=0D >> @@ -36,6 +36,10 @@=0D >> * C expressions can now use _Alignof, and C++ expressions can now use=0D >> alignof.=0D >> =0D >> +* The File I/O remote protocole extension now allows opening special fi= les such=0D >> + as FIFOs or character devices. Common file types are now be returned = by stat=0D >> + and fstat.=0D > A couple typos in there "protocole", "are now be".=0D >=0D >=0D > I suggest this:=0D >=0D > * The File I/O remote protocol feature has been extended to allow=0D > opening and stating special files such as FIFOs and character=0D > devices. Previously only regular files and GDB's console were=0D > supported.=0D >=0D =0D Ok.=0D =0D >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo=0D >> index 91ec219958..54b9ff6253 100644=0D >> --- a/gdb/doc/gdb.texinfo=0D >> +++ b/gdb/doc/gdb.texinfo=0D >> @@ -40986,7 +40986,8 @@ range of values.=0D >> @table @code=0D >> =0D >> @item st_dev=0D >> -A value of 0 represents a file, 1 the console.=0D >> +A value of 0 represents a file, 1 GDB's console and 2 a special file=0D >> +(@code{st_mode} gives further details on the file type).=0D > I think:=0D >=0D > - "0 represents a file"=0D > + "0 represents a regular file"=0D >=0D > would help clarify this a bit.=0D =0D Ok.=0D =0D >> =0D >> @item st_ino=0D >> No valid meaning for the target. Transmitted unchanged.=0D >> @@ -41073,8 +41074,12 @@ All values are given in hexadecimal representat= ion.=0D >> All values are given in octal representation.=0D >> =0D >> @smallexample=0D >> + S_IFSOCK 0140000=0D >> S_IFREG 0100000=0D >> + S_IFBLK 060000=0D >> S_IFDIR 040000=0D >> + S_IFCHR 020000=0D >> + S_IFIFO 010000=0D >> S_IRUSR 0400=0D >> S_IWUSR 0200=0D >> S_IXUSR 0100=0D >> diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c=0D >> index 313da642ea..168590245e 100644=0D >> --- a/gdb/remote-fileio.c=0D >> +++ b/gdb/remote-fileio.c=0D >> @@ -885,16 +885,9 @@ remote_fileio_func_stat (remote_target *remote, cha= r *buf)=0D >> remote_fileio_return_errno (remote, -1);=0D >> return;=0D >> }=0D >> - /* Only operate on regular files and directories. */=0D >> - if (!ret && !S_ISREG (st.st_mode) && !S_ISDIR (st.st_mode))=0D >> - {=0D >> - remote_fileio_reply (remote, -1, FILEIO_EACCES);=0D >> - return;=0D >> - }=0D >=0D > What happens if we stat/open some kind of unsupported file type?=0D > Do we end up with st_mode =3D=3D 0 and report success anyway, or is=0D > something else catching it and returning FILEIO_EACCES or some such?=0D >=0D =0D Yes, bits SFMT of st_mode end up with everything 0 and it doesn't fail.=0D It's like not knowing what kind of file it exactly is, but still get=0D other values.=0D =0D >> diff --git a/gdb/testsuite/gdb.base/fileio.c b/gdb/testsuite/gdb.base/fi= leio.c=0D >> index 7f482a34d3..c1902f5cc8 100644=0D >> --- a/gdb/testsuite/gdb.base/fileio.c=0D >> +++ b/gdb/testsuite/gdb.base/fileio.c=0D > Can you say something about how you tested this?=0D >=0D > I tried it here, and with the patch, the test as is fails with plain=0D > "make check", i.e,. when testing with the native target:=0D >=0D > $ make check TESTS=3D"gdb.base/fileio.exp"=0D > Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.base/fileio.e= xp ...=0D > FAIL: gdb.base/fileio.exp: Stat a file=0D > FAIL: gdb.base/fileio.exp: Stat a nonexistant file returns ENOENT=0D > FAIL: gdb.base/fileio.exp: Stat a directory=0D > FAIL: gdb.base/fileio.exp: Fstat an open file=0D > FAIL: gdb.base/fileio.exp: Fstat a directory=0D > FAIL: gdb.base/fileio.exp: Fstat standard output=0D > FAIL: gdb.base/fileio.exp: Fstat standard input=0D > FAIL: gdb.base/fileio.exp: Fstat standard error=0D >=0D > We have to make sure that continues passing, as that helps=0D > ensure the testcase's program is written in a portable fashion.=0D >=0D > I think part of it is that the testcase is now assuming st_dev=0D > contains the File I/O 0/1/2 values, for regular/console/special.=0D >=0D > I think the best course of action here is to add something like=0D > this:=0D >=0D > static int=0D > is_st_dev (int actual, int expected)=0D > {=0D > #ifdef TESTING_RSP=0D > return actual =3D=3D expected;=0D > #else=0D > return 1;=0D > #endif=0D > }=0D >=0D > With the .exp file passing additional_flags=3D-DTESTING_RSP if testing=0D > against a remote protocol target.=0D >=0D > And then use it like:=0D >=0D > printf ("fstat 3: ret =3D %d, errno =3D %d %s\n", ret, errno,=0D > - st.st_dev =3D=3D 2 && S_ISDIR (st.st_mode) ? "OK" : "");= =0D > + is_st_dev (st.st_dev, 2) && S_ISDIR (st.st_mode) ? "OK" := "");=0D >=0D >=0D > The .exp can tell when we're testing against a remote protocol=0D > target with:=0D >=0D > if {[target_info gdb_protocol] =3D=3D "remote"=0D > || [target_info gdb_protocol] =3D=3D "extended-remote"} {=0D =0D Ok. I see. I tested with our stubs running on a Raspberry Pi by defining=0D a board .exp file, inspired by board definitions sid and gdbserver.=0D =0D >> =0D >> #define STRING "Hello World"=0D >> =0D >> @@ -292,7 +301,8 @@ test_stat (void)=0D >> ret =3D stat (OUTDIR FILENAME, &st);=0D >> if (!ret)=0D >> printf ("stat 1: ret =3D %d, errno =3D %d %s\n", ret, errno,=0D >> - st.st_size =3D=3D 11 ? "OK" : "");=0D >> + st.st_dev =3D=3D 0 && S_ISREG (st.st_mode) && st.st_size =3D=3D 11= ?=0D >> + "OK" : "");=0D >> else=0D >> printf ("stat 1: ret =3D %d, errno =3D %d\n", ret, errno);=0D >> stop ();=0D >> @@ -311,8 +321,20 @@ test_stat (void)=0D >> /* Nonexistant file */=0D >> errno =3D 0;=0D >> ret =3D stat (NONEXISTANT, &st);=0D >> - printf ("stat 4: ret =3D %d, errno =3D %d %s\n", ret, errno,=0D >> - strerrno (errno));=0D >> + if (!ret)=0D >> + printf ("stat 4: ret =3D %d, errno =3D %d %s\n", ret, errno,=0D >> + strerrno (errno));=0D >> + else=0D >> + printf ("stat 1: ret =3D %d, errno =3D %d\n", ret, errno);=0D > Do we want to print errno in the !ret case? That indicates the=0D > stat call succeeded (even though we expect it to fail).=0D > Might be that helps simplify the .exp, I haven't checked.=0D >=0D > But at least the strerrno call should be on the else=0D > branch, as that's the branch where errno is meaninful, and=0D > the .exp expects it:=0D >=0D > FAIL: gdb.base/fileio.exp: Stat a nonexistant file returns ENOENT=0D >=0D > Did this pass against your stub?=0D =0D Yes, here are logs of GDB's execution on this case:=0D =0D > Continuing.=0D > stat 4: ret =3D -1, errno =3D 2 ENOENT=0D >=0D > Breakpoint 2, stop () at=0D /home/julio/gnu/gdb/build-arm-none-eabi/gdb/testsuite/../../../gdb/testsuit= e/gdb.base/fileio.c:84=0D > 84=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 static void stop (void) {}=0D =0D >=0D > Also, here both branches should say "stat 4", I think?=0D >=0D >=0D >> + stop ();=0D >> + /* Special file: directory */=0D >> + errno =3D 0;=0D >> + ret =3D stat (OUTDIR DIRECTORY, &st);=0D >> + if (!ret)=0D >> + printf ("stat 5: ret =3D %d, errno =3D %d %s\n", ret, errno,=0D >> + st.st_dev =3D=3D 2 && S_ISDIR (st.st_mode) ? "OK" : "");=0D >> + else=0D >> + printf ("stat 1: ret =3D %d, errno =3D %d\n", ret, errno);=0D > Shouldn't both branches here say "stat 5" ?=0D =0D Excessive copy/paste, sorry.=0D =0D >> void=0D >> diff --git a/gdb/testsuite/gdb.base/fileio.exp b/gdb/testsuite/gdb.base/= fileio.exp=0D >> index bc409c26aa..234f304ac7 100644=0D >> --- a/gdb/testsuite/gdb.base/fileio.exp=0D >> +++ b/gdb/testsuite/gdb.base/fileio.exp=0D >> @@ -31,7 +31,7 @@ if {[is_remote host]} {=0D >> =0D >> if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \=0D >> executable \=0D >> - [list debug "additional_flags=3D-DOUTDIR=3D\"$outdir/\""]] !=3D "" = } {=0D >> + [list debug "additional_flags=3D-DOUTDIR=3D\"$outdir/\" [target_i= nfo fileio,cflags]" "ldflags=3D[target_info fileio,ldflags]"]] !=3D "" } {= =0D > I couldn't tell what's this change for? Why did you need it?=0D =0D I couldn't find any other way of adding some CFLAGS and LDFLAGS to the=0D call to the cross-compiler to link against the libc using File IOs, to=0D add target-specific compilation flags, etc. For example, in my case:=0D =0D > set_board_info fileio,cflags "--specs=3D$sdk/Alpha.specs=0D -mfloat-abi=3Dhard -mfpu=3Dvfp -march=3Darmv6zk -mtune=3Darm1176jzf-s"=0D > set_board_info fileio,ldflags "-Wl,-T$sdk/link.ld"=0D =0D >=0D >> untested "failed to compile"=0D >> return -1=0D >> }=0D >> @@ -136,6 +136,10 @@ gdb_test continue \=0D >> "Continuing\\..*close 2:.*EBADF$stop_msg" \=0D >> "Closing an invalid file descriptor returns EBADF"=0D >> =0D >> +# Prepare some different file types for stat and fstat.=0D >> +set filetype_directory [file join $outdir "directory.fileio.test"]=0D >> +file mkdir $filetype_directory=0D >> +=0D > Please use remote_exec like the other mkdir calls in the file.=0D >=0D =0D Ok.=0D =0D -- =0D Julio Guerra=0D Co-founder & CTO of Farjump=0D Mobile: +33 618 644 164=0D LinkedIn: https://linkedin.com/in/guerrajulio=0D Slack: farjump.slack.com=0D =0D