[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 1 May 2018 14:49:00 -0700
From: Andre McCurdy <armccurdy@...il.com>
To: musl@...ts.openwall.com
Cc: Patrick Oppenlander <patrick.oppenlander@...il.com>
Subject: Re: Some questions
On Tue, May 1, 2018 at 10:35 AM, Rich Felker <dalias@...c.org> wrote:
> 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.
This seems to fail when compiling src/thread/arm/__set_thread_area.c:
{standard input}: Assembler messages:
{standard input}:45: Error: invalid constant (f0005) after fixup
make: *** [obj/src/thread/arm/__set_thread_area.o] Error 1
Without the patch, __set_thread_area() effectively compiles to:
__set_thread_area:
push {r7, lr}
ldr r7, .L2
pop {r7, pc}
.L2:
.word 983045
With the patch:
__set_thread_area:
push {r7, lr}
add r7, sp, #0
mov r3,r7 ; mov r7,#983045 ; svc 0 ; mov r7,r3
pop {r7, pc}
ie the immediate value 0xf0005 can't be loaded directly into r7 with a
single Thumb2 mov instruction.
I tried a quick test to replace the single mov instruction in
__asm_syscall() with a movw + movt pair:
#define __asm_syscall(...) do { \
__asm__ __volatile__ ( "mov %1,r7 ; movw r7,%2 & 0xffff ; movt
r7,%2 >> 16 ; svc 0 ; mov r7,%1" \
: "=r"(r0), "=&r"((int){0}) : __VA_ARGS__ : "memory"); \
return r0; \
} while (0)
It fixes __set_thread_area() but causes a new error in syscall() as
the syscall number is passed to __asm_syscall() as a variable rather
than an immediate:
{standard input}: Assembler messages:
{standard input}:75: Error: constant expression expected -- `movw
r7,r6&0xffff'
{standard input}:75: Error: constant expression expected -- `movt r7,r6>>16'
make: *** [obj/src/misc/syscall.o] Error 1
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.
