Re: Some questions

[Rich Felker <dalias@libc.org>]

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.

Rich