Re: Some questions

[Rich Felker <dalias@libc.org>]

[-- Attachment #1: Type: text/plain, Size: 3961 bytes --]

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@libc.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

[-- Attachment #2: thumb-syscall.diff --]
[-- Type: text/plain, Size: 3417 bytes --]

diff --git a/arch/arm/syscall_arch.h b/arch/arm/syscall_arch.h
index 6023303..30863eb 100644
--- a/arch/arm/syscall_arch.h
+++ b/arch/arm/syscall_arch.h
@@ -3,74 +3,89 @@
 ((union { long long ll; long l[2]; }){ .ll = x }).l[1]
 #define __SYSCALL_LL_O(x) 0, __SYSCALL_LL_E((x))
 
+#ifdef __thumb__
+
+#define __ASM____R7__
+#define R7_OPERAND "ri"(r7)
+#define __asm_syscall(...) do { \
+	__asm__ __volatile__ ( "mov %1,r7 ; mov r7,%2 ; svc 0 ; mov r7,%1" \
+	: "=r"(r0), "=&r"((int){0}) : __VA_ARGS__ : "memory"); \
+	return r0; \
+	} while (0)
+
+#else
+
+#define __ASM____R7__ __asm__("r7")
+#define R7_OPERAND "r"(r7)
 #define __asm_syscall(...) do { \
 	__asm__ __volatile__ ( "svc 0" \
 	: "=r"(r0) : __VA_ARGS__ : "memory"); \
 	return r0; \
 	} while (0)
+#endif
 
 static inline long __syscall0(long n)
 {
-	register long r7 __asm__("r7") = n;
+	register long r7 __ASM____R7__ = n;
 	register long r0 __asm__("r0");
-	__asm_syscall("r"(r7));
+	__asm_syscall(R7_OPERAND);
 }
 
 static inline long __syscall1(long n, long a)
 {
-	register long r7 __asm__("r7") = n;
+	register long r7 __ASM____R7__ = n;
 	register long r0 __asm__("r0") = a;
-	__asm_syscall("r"(r7), "0"(r0));
+	__asm_syscall(R7_OPERAND, "0"(r0));
 }
 
 static inline long __syscall2(long n, long a, long b)
 {
-	register long r7 __asm__("r7") = n;
+	register long r7 __ASM____R7__ = n;
 	register long r0 __asm__("r0") = a;
 	register long r1 __asm__("r1") = b;
-	__asm_syscall("r"(r7), "0"(r0), "r"(r1));
+	__asm_syscall(R7_OPERAND, "0"(r0), "r"(r1));
 }
 
 static inline long __syscall3(long n, long a, long b, long c)
 {
-	register long r7 __asm__("r7") = n;
+	register long r7 __ASM____R7__ = n;
 	register long r0 __asm__("r0") = a;
 	register long r1 __asm__("r1") = b;
 	register long r2 __asm__("r2") = c;
-	__asm_syscall("r"(r7), "0"(r0), "r"(r1), "r"(r2));
+	__asm_syscall(R7_OPERAND, "0"(r0), "r"(r1), "r"(r2));
 }
 
 static inline long __syscall4(long n, long a, long b, long c, long d)
 {
-	register long r7 __asm__("r7") = n;
+	register long r7 __ASM____R7__ = n;
 	register long r0 __asm__("r0") = a;
 	register long r1 __asm__("r1") = b;
 	register long r2 __asm__("r2") = c;
 	register long r3 __asm__("r3") = d;
-	__asm_syscall("r"(r7), "0"(r0), "r"(r1), "r"(r2), "r"(r3));
+	__asm_syscall(R7_OPERAND, "0"(r0), "r"(r1), "r"(r2), "r"(r3));
 }
 
 static inline long __syscall5(long n, long a, long b, long c, long d, long e)
 {
-	register long r7 __asm__("r7") = n;
+	register long r7 __ASM____R7__ = n;
 	register long r0 __asm__("r0") = a;
 	register long r1 __asm__("r1") = b;
 	register long r2 __asm__("r2") = c;
 	register long r3 __asm__("r3") = d;
 	register long r4 __asm__("r4") = e;
-	__asm_syscall("r"(r7), "0"(r0), "r"(r1), "r"(r2), "r"(r3), "r"(r4));
+	__asm_syscall(R7_OPERAND, "0"(r0), "r"(r1), "r"(r2), "r"(r3), "r"(r4));
 }
 
 static inline long __syscall6(long n, long a, long b, long c, long d, long e, long f)
 {
-	register long r7 __asm__("r7") = n;
+	register long r7 __ASM____R7__ = n;
 	register long r0 __asm__("r0") = a;
 	register long r1 __asm__("r1") = b;
 	register long r2 __asm__("r2") = c;
 	register long r3 __asm__("r3") = d;
 	register long r4 __asm__("r4") = e;
 	register long r5 __asm__("r5") = f;
-	__asm_syscall("r"(r7), "0"(r0), "r"(r1), "r"(r2), "r"(r3), "r"(r4), "r"(r5));
+	__asm_syscall(R7_OPERAND, "0"(r0), "r"(r1), "r"(r2), "r"(r3), "r"(r4), "r"(r5));
 }
 
 #define VDSO_USEFUL