Fix usage with musl libc.

Fix usage with musl libc.

BUG=

[Kylie McClain, 2016-03-13 15:36:17]

Ping?

[Mark Seaborn, 2016-03-13 19:59:48]

mseaborn@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2016-03-13 19:59:49]

On 2016/03/13 15:36:17, Kylie McClain wrote:
> Ping?

FYI, you need to make Rietveld send e-mail (with "Publish+Mail Comments") to get
a code review.  Just uploading a change to Rietveld is not enough.

https://codereview.chromium.org/1743093002/diff/1/linux_syscall_support.h
File linux_syscall_support.h (right):

https://codereview.chromium.org/1743093002/diff/1/linux_syscall_support.h#new...
linux_syscall_support.h:104: #define _GNU_SOURCE /* musl libc defines loff_t
only with _GNU_SOURC */
Typo: "SOURC"

I'm not sure that defining _GNU_SOURCE here is a good idea, because:
 * it will leak to files #included after linux_syscall_support.h
 * it won't work if fcntl.h was #included earlier

What's your use case?  i.e. What are you building that uses
linux_syscall_support.h?

https://codereview.chromium.org/1743093002/diff/1/linux_syscall_support.h#new...
linux_syscall_support.h:1062: #define __NR_pread __NR_pread64
Please explain what this fixes.  What's your test case?

[KylieMcClain, 2016-03-18 12:45:09]

On 2016/03/13 19:59:49, Mark Seaborn wrote:
> On 2016/03/13 15:36:17, Kylie McClain wrote:
> > Ping?
> 
> FYI, you need to make Rietveld send e-mail (with "Publish+Mail Comments") to
get
> a code review.  Just uploading a change to Rietveld is not enough.

Sorry about that, I'm not very acquainted with Rietveld, I'm used to Gerrit just
spamming with emails. :)
 
>
https://codereview.chromium.org/1743093002/diff/1/linux_syscall_support.h#new...
> linux_syscall_support.h:104: #define _GNU_SOURCE /* musl libc defines loff_t
> only with _GNU_SOURC */
> Typo: "SOURC"

Fixed.
 
> I'm not sure that defining _GNU_SOURCE here is a good idea, because:
>  * it will leak to files #included after linux_syscall_support.h
>  * it won't work if fcntl.h was #included earlier

Perhaps _GNU_SOURCE should be added to the top? Or should it be undefined after
including fcntl.h... I'm not really sure how to handle it being included
earlier, either.

> What's your use case?  i.e. What are you building that uses
> linux_syscall_support.h?

I'm currently working on getting Firefox to build without upstream patching on
musl libc; it runs fine with some non-upstreamed patches. Some musl issues with
Firefox's build are due to breakpad, and in turn, linux_syscall_support.

>
https://codereview.chromium.org/1743093002/diff/1/linux_syscall_support.h#new...
> linux_syscall_support.h:1062: #define __NR_pread __NR_pread64
> Please explain what this fixes.  What's your test case?
musl defines __NR_pread64 on all of its targets. The only target which mentions
__NR_pread is x32, which then does `#define __NR_pread __NR_pread64`. So, on
musl, __NR_pread64 is what definition should be depended on. Asking on #musl, I
was told that there's no `pread` syscall on Linux anyway, only preadv and
pread64. So I suppose it's a glibc-ism...?

Thanks for reviewing. :)

[Kylie McClain, 2016-03-18 12:48:09]

(eh, sorry about that, I logged in from the wrong Google account)

[Mark Seaborn, 2016-03-18 15:11:52]

On 18 March 2016 at 05:45, <huntermcclainer@gmail.com> wrote:

> On 2016/03/13 19:59:49, Mark Seaborn wrote:
> > I'm not sure that defining _GNU_SOURCE here is a good idea, because:
> > * it will leak to files #included after linux_syscall_support.h
> > * it won't work if fcntl.h was #included earlier
>
> Perhaps _GNU_SOURCE should be added to the top? Or should it be undefined
> after
> including fcntl.h... I'm not really sure how to handle it being included
> earlier, either.
>

Defining _GNU_SOURCE in linux_syscall_support.h just doesn't work.  You'd
need to define it in the build system of the project that uses LSS.

Alternatively, you could change linux_syscall_support.h to avoid using
loff_t, if loff_t is non-standard.  Isn't int64_t equivalent?


> What's your use case? i.e. What are you building that uses
> > linux_syscall_support.h?
>
> I'm currently working on getting Firefox to build without upstream
> patching on
> musl libc; it runs fine with some non-upstreamed patches. Some musl issues
> with
> Firefox's build are due to breakpad, and in turn, linux_syscall_support.
>
> >
>
>
https://codereview.chromium.org/1743093002/diff/1/linux_syscall_support.h#new...
> > linux_syscall_support.h:1062: #define __NR_pread __NR_pread64
> > Please explain what this fixes. What's your test case?
> musl defines __NR_pread64 on all of its targets. The only target which
> mentions
> __NR_pread is x32, which then does `#define __NR_pread __NR_pread64`. So,
> on
> musl, __NR_pread64 is what definition should be depended on. Asking on
> #musl, I
> was told that there's no `pread` syscall on Linux anyway, only preadv and
> pread64. So I suppose it's a glibc-ism...?
>

OK, but what code references pread and not pread64?  I don't see any
references in linux_syscall_support.h.  Are they elsewhere?

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kylie McClain, 2016-03-21 19:43:43]

> Defining _GNU_SOURCE in linux_syscall_support.h just doesn't work.  You'd
> need to define it in the build system of the project that uses LSS.
> 
> Alternatively, you could change linux_syscall_support.h to avoid using
> loff_t, if loff_t is non-standard.  Isn't int64_t equivalent?

Fixed, loff_t was replaced with int64_t (which you're right, are equivalent)

> OK, but what code references pread and not pread64?  I don't see any
> references in linux_syscall_support.h.  Are they elsewhere?

I think it's in breakpad? Building breakpad with the `#define __NR_pread` lines
removed
gives me this: http://ix.io/uwR (note that this is with my breakpad patches in
https://codereview.chromium.org/1726163002/ )

[Mark Seaborn, 2016-03-22 19:31:09]

On 21 March 2016 at 12:43, <somasissounds@gmail.com> wrote:

> > Defining _GNU_SOURCE in linux_syscall_support.h just doesn't work. You'd
> > need to define it in the build system of the project that uses LSS.
> >
> > Alternatively, you could change linux_syscall_support.h to avoid using
> > loff_t, if loff_t is non-standard. Isn't int64_t equivalent?
>
> Fixed, loff_t was replaced with int64_t (which you're right, are
> equivalent)
>
> > OK, but what code references pread and not pread64? I don't see any
> > references in linux_syscall_support.h. Are they elsewhere?
>
> I think it's in breakpad? Building breakpad with the `#define __NR_pread`
> lines
> removed
> gives me this: http://ix.io/uwR (note that this is with my breakpad
> patches in
> https://codereview.chromium.org/1726163002/ )
>

So you posted:

In file included from
> src/client/linux/crash_generation/crash_generation_client.cc:40:0:
> ./src/third_party/lss/linux_syscall_support.h: In function 'ssize_t
> sys_pread64(int, void*, size_t, off_t)':
> ./src/third_party/lss/linux_syscall_support.h:1973:20: error: '__NR_pread'
> was not declared in this scope
>              : "0" (__NR_##name) LSS_BODY_ARG##nr(__VA_ARGS__)
>     \
>                     ^
> ./src/third_party/lss/linux_syscall_support.h:1978:7: note: in expansion
> of macro '_LSS_BODY'
>        _LSS_BODY(nr, type, name, uintptr_t, ## args)
>        ^
> ./src/third_party/lss/linux_syscall_support.h:3870:7: note: in expansion
> of macro 'LSS_BODY'
>        LSS_BODY(4, ssize_t, pread64, LSS_SYSCALL_ARG(f),
> LSS_SYSCALL_ARG(b),
>        ^


The point in Breakpad where this happens is just where Breakpad #includes
linux_syscall_support.h (crash_generation_client.cc:40).

And linux_syscall_support.h is referring to "pread64", not "pread", as
shown above.

I don't see any references to pread in Breakpad that would cause this
problem.

Can you debug this further and figure out why this is turning into a
reference to __NR_pread instead of __NR_pread64?

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kylie McClain, 2016-04-06 20:48:03]

> Can you debug this further and figure out why this is turning into a
> reference to __NR_pread instead of __NR_pread64?

I'm really not sure as to what is causing that either. musl internals don't use
__NR_pread either.

[Mark Seaborn, 2016-04-06 21:16:53]

On 6 April 2016 at 13:48, <somasissounds@gmail.com> wrote:

> > Can you debug this further and figure out why this is turning into a
> > reference to __NR_pread instead of __NR_pread64?
>
> I'm really not sure as to what is causing that either. musl internals
> don't use
> __NR_pread either.
>

OK, let's try breaking this down to debug this...

With your Musl toolchain, try running:

  gcc -Wp,-dD linux_syscall_support.h -E -o -

"-E" with "-Wp,-dD" gives the output from the preprocessor, with the
#defines left in.  Look for references to "pread64" and "pread" in there.
This should indicate whether some file is doing something like "#define
__NR_pread64 __NR_pread", and if so which file.

With Ubuntu/glibc, I get:

$ gcc -Wp,-dD linux_syscall_support.h -E -o - | grep pread
#define __NR_pread64 17
#define __NR_preadv 295
#define SYS_pread64 __NR_pread64
#define SYS_preadv __NR_preadv
extern ssize_t pread (int __fd, void *__buf, size_t __nbytes,
    static inline ssize_t sys_pread64(int f, void *b, size_t c, loff_t o) {

What's the equivalent output for your Musl toolchain?

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kylie McClain, 2016-04-06 21:32:21]

> What's the equivalent output for your Musl toolchain?

$ x86_64-pc-linux-musl-cc -Wp,-dD linux_syscall_support.h -E -o - | grep pread
#define __NR_pread64 17
#define __NR_preadv 295
#define SYS_pread64 17
#define SYS_preadv 295
ssize_t pread(int, void *, size_t, off_t);
#define __NR_pread64 17
#define __NR_preadv 295
    static inline ssize_t sys_pread64(int f, void *b, size_t c, int64_t o) {


Perhaps it's something funny about that ssize_t pread() definition?
http://git.musl-libc.org/cgit/musl/tree/include/unistd.h#n48

[Mark Seaborn, 2016-04-06 21:44:24]

On 6 April 2016 at 14:32, <somasissounds@gmail.com> wrote:

> > What's the equivalent output for your Musl toolchain?
>
> $ x86_64-pc-linux-musl-cc -Wp,-dD linux_syscall_support.h -E -o - | grep
> pread
> #define __NR_pread64 17
> #define __NR_preadv 295
> #define SYS_pread64 17
> #define SYS_preadv 295
> ssize_t pread(int, void *, size_t, off_t);
> #define __NR_pread64 17
> #define __NR_preadv 295
> static inline ssize_t sys_pread64(int f, void *b, size_t c, int64_t o) {
>

Let's get some more context.  What is the output with "grep -C 3 pread"?

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kylie McClain, 2016-04-07 01:20:09]

> Let's get some more context.  What is the output with "grep -C 3 pread"?


$ x86_64-pc-linux-musl-cc -Wp,-dD linux_syscall_support.h -E -o - | grep -C 3
pread
#define __NR_rt_sigprocmask 14
#define __NR_rt_sigreturn 15
#define __NR_ioctl 16
#define __NR_pread64 17
#define __NR_pwrite64 18
#define __NR_readv 19
#define __NR_writev 20
--
#define __NR_dup3 292
#define __NR_pipe2 293
#define __NR_inotify_init1 294
#define __NR_preadv 295
#define __NR_pwritev 296
#define __NR_rt_tgsigqueueinfo 297
#define __NR_perf_event_open 298
--
#define SYS_rt_sigprocmask 14
#define SYS_rt_sigreturn 15
#define SYS_ioctl 16
#define SYS_pread64 17
#define SYS_pwrite64 18
#define SYS_readv 19
#define SYS_writev 20
--
#define SYS_dup3 292
#define SYS_pipe2 293
#define SYS_inotify_init1 294
#define SYS_preadv 295
#define SYS_pwritev 296
#define SYS_rt_tgsigqueueinfo 297
#define SYS_perf_event_open 298
--

ssize_t read(int, void *, size_t);
ssize_t write(int, const void *, size_t);
ssize_t pread(int, void *, size_t, off_t);
ssize_t pwrite(int, const void *, size_t, off_t);

int chown(const char *, uid_t, gid_t);
--
#define __NR_rt_sigprocmask 14
#define __NR_rt_sigreturn 15
#define __NR_ioctl 16
#define __NR_pread64 17
#define __NR_pwrite64 18
#define __NR_readv 19
#define __NR_writev 20
--
#define __NR_dup3 292
#define __NR_pipe2 293
#define __NR_inotify_init1 294
#define __NR_preadv 295
#define __NR_pwritev 296
#define __NR_rt_tgsigqueueinfo 297
#define __NR_perf_event_open 298
--
  }


    static inline ssize_t sys_pread64(int f, void *b, size_t c, int64_t o) {
      long long __res; __asm__ __volatile__( "movq %5,%%r10;" "syscall\n" : "=a"
(__res) : "0" (
# 3867 "linux_syscall_support.h" 3 4
     17

[Mark Seaborn, 2016-04-07 04:08:44]

On 6 April 2016 at 18:20, <somasissounds@gmail.com> wrote:

> > Let's get some more context. What is the output with "grep -C 3 pread"?
>
>
> $ x86_64-pc-linux-musl-cc -Wp,-dD linux_syscall_support.h -E -o - | grep
> -C 3
>

OK, thanks for checking.

I presume linux_syscall_support.h works OK on its own then.  It just fails
when #included in the context of compiling
src/client/linux/crash_generation/crash_generation_client.cc from Breakpad,
right?  So you can try the same "-Wp,-dD -E" + grep with that Breakpad file
and it should show the source of the problem.

Having looked into it, I think the cause is this:

glibc does this, which works OK:
/usr/include/unistd.h:#   define pread pread64

But Musl has the #define the other way around:
include/unistd.h:#define pread64 pread

So linux_syscall_support.h's reference to __NR_##pread64 ends up being
turned into __NR_pread.

I'm not sure whether it's OK for Musl to be doing that #define (or glibc
either for that matter).  Maybe Musl could be changed to define pread64()
as an inline function that calls pread(), or as an alias using
__attribute__((alias))?

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.