[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20180501173535.GT1392@brightrain.aerifal.cx>
Date: Tue, 1 May 2018 13:35:35 -0400
From: Rich Felker <dalias@...c.org>
To: Patrick Oppenlander <patrick.oppenlander@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: Some questions
On Tue, May 01, 2018 at 11:52:33AM -0400, Rich Felker wrote:
> On Tue, May 01, 2018 at 12:34:13PM +1000, Patrick Oppenlander wrote:
> > On Tue, May 1, 2018 at 1:31 AM, Rich Felker <dalias@...c.org> wrote:
> > > On Mon, Apr 30, 2018 at 03:29:39PM +1000, Patrick Oppenlander wrote:
> > >> Actually, my biggest issue with getcwd is that it allocates a PATH_MAX
> > >> sized buffer on the stack. That's painful on deeply embedded stuff.
> > >
> > > That's unrelated, and could/should be fixed by the attached patch I
> > > think.
> >
> > Unfortunately that fails to build on arm with:
> >
> > src/unistd/getcwd.c: In function 'getcwd':
> > src/unistd/getcwd.c:25:1: error: r7 cannot be used in asm here
>
> Then that's a bug we need to fix or work around elsewhere.
> Non-arch-specific source files can't be constrained not to use
> perfectly valid C constructs because gcc breaks on them for particular
> archs.
>
> I believe it's related to the thumb+framepointer issue that was raised
> a while back, but I forget how that ended and if we ever solved it.
>
> > I was also having a go at resolving the stack & the buffer size issue
> > and came up with the attached (untested) patch.
> >
> > Patrick
>
> > diff --git a/src/unistd/getcwd.c b/src/unistd/getcwd.c
> > index 103fbbb5..306dbc4f 100644
> > --- a/src/unistd/getcwd.c
> > +++ b/src/unistd/getcwd.c
> > @@ -3,17 +3,10 @@
> > #include <limits.h>
> > #include <string.h>
> > #include "syscall.h"
> > +#include "libc.h"
> >
> > -char *getcwd(char *buf, size_t size)
> > +static char *do_getcwd(char *buf, size_t size)
> > {
> > - char tmp[PATH_MAX];
> > - if (!buf) {
> > - buf = tmp;
> > - size = PATH_MAX;
> > - } else if (!size) {
> > - errno = EINVAL;
> > - return 0;
> > - }
> > long ret = syscall(SYS_getcwd, buf, size);
> > if (ret < 0)
> > return 0;
> > @@ -21,5 +14,37 @@ char *getcwd(char *buf, size_t size)
> > errno = ENOENT;
> > return 0;
> > }
> > - return buf == tmp ? strdup(buf) : buf;
> > + return buf;
> > +}
> > +
> > +static char *getcwd_glibc(size_t size)
> > +{
> > + char tmp[PATH_MAX];
> > + if (!do_getcwd(tmp, sizeof tmp))
> > + return 0;
> > + size_t len = strlen(tmp) + 1;
> > + if (!size)
> > + size = len;
> > + else if (size < len) {
> > + errno = ERANGE;
> > + return 0;
> > + }
> > + char *buf = malloc(size);
> > + if (!buf) {
> > + errno = ENOMEM;
> > + return 0;
> > + }
> > + memcpy(buf, tmp, len);
> > + return buf;
> > +}
> > +
> > +char *getcwd(char *buf, size_t size)
> > +{
> > + if (!buf)
> > + return getcwd_glibc(size);
> > + if (!size) {
> > + errno = EINVAL;
> > + return 0;
> > + }
> > + return do_getcwd(buf, size);
> > }
>
> This isn't acceptable. It makes the code much larger (at the source
> level) and harder to read, and the only reason it works is failure of
> gcc to optimize heavily. It could just as easily still end up using
> the full PATH_MAX space on the stack, if gcc inlines and hoists stuff,
> or if gcc wanted to be really awful it could still end up using a
> frame pointer.
>
> Let's look back at the framepointer mess and see if there's a way to
> get gcc not to break. If not we may need to skip inline syscalls and
> call out to the extern __syscall when building for thumb, but I'd
> really rather not have to do that.
Looking back, it seems where we left it is just that you need to make
sure frame pointer is disabled if building as thumb. But that's not
reliable because gcc forcibly re-enables frame pointer (including
frame pointer ABI constraints, which it doesn't need to) if you use a
VLA or alloca.
I'm considering applying the attached patch, which would make it so
VLAs don't break thumb syscalls and eliminate the need to force frame
pointer off when building as thumb. This is all a workaround for gcc
being wrong about not letting you use r7, but it seems reasonable and
non-invasive. It just omits r7 from the constraints and uses a temp
register to save/restore it.
Rich
View attachment "thumb-syscall.diff" of type "text/plain" (3418 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
