935962 - [OS.File] Firefox does not restore tabs on NetBSD

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Martin Husemann]

Attachment: Use proper exported libc names on NetBSD

Similar effect to Bug 861829.

The NetBSD libc (mostly for historic reasons) uses a sheme to provide binary compatibility with old binaries that causes newer functions to be exported under a "renamed" name. For example readdir(3) changed with NetBSD 3.0, the new exported name now is __readdir30.

Libffi does not know about the C header magic involved, so OS.File needs to know the new names and use those. Otherwise lots of things do not work.

It would be good to generate a list of these renamed functions at build time (compile a C program and look at nm output for example), but a first step is to fix the names in a hardcoded way. The attached patch does this and works.

It also fixes the "st_size" field in struct stat, which is not size_t but off_t in NetBSD.

Comment 1

[Martin Husemann]

Attachment: Ryo ONODERA points out another st_size hardcoded type which needs to be off_t on NetBSD

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 828625 [details] [diff] [review]
Use proper exported libc names on NetBSD

Review of attachment 828625 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with a trivial layout change.
Could you file a bug about using nm in the build system? This sounds like a very good idea.

::: toolkit/components/osfile/modules/osfile_unix_back.jsm
@@ +165,5 @@
>           }
>  
> +         if (OS.Constants.Sys.Name == "NetBSD")
> +           stat.add_field_at(Const.OSFILE_OFFSETOF_STAT_ST_SIZE,
> +                        "st_size", Type.off_t.implementation);

Nit:
if (...) {
  ...
} else {
  ...
}

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 828628 [details] [diff] [review]
Ryo ONODERA points out another st_size hardcoded type which needs to be off_t on NetBSD

Review of attachment 828628 [details] [diff] [review]:
-----------------------------------------------------------------

For this one, I'd prefer if you created in osfile_unix_back.jsm a "proper" instance of Type for file size.

Comment 4

[Martin Husemann]

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #3)

> For this one, I'd prefer if you created in osfile_unix_back.jsm a "proper"
> instance of Type for file size.

Just to make sure I don't misunderstand: you mean "in osfile_shared_allthreads.jsm" ?

Comment 5

[Martin Husemann]

Attachment: v2: create a special type for stat.st_size and deal with NetBSD libc symbol names

Comment 6

[Martin Husemann]

Attachment: v3: create a special type for stat.st_size and deal with NetBSD libc symbol names

oops, missed a few { } previously

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 829163 [details] [diff] [review]
v2: create a special type for stat.st_size and deal with NetBSD libc symbol names

Review of attachment 829163 [details] [diff] [review]:
-----------------------------------------------------------------

I really meant osfile_unix_back.jsm. We define many types specific to Unix in that file, e.g. Type.fd, Type.mode_t, etc. You should define Type.stat_size_t there.

::: toolkit/components/osfile/modules/osfile_shared_allthreads.jsm
@@ +740,5 @@
> +    new IntType("off_t", ctypes.off_t, true);
> +} else {
> +  Type.stat_size_t =
> +    new IntType("size_t", ctypes.size_t, true);
> +}

Since stat_size_t is defined from off_t (respectively size_t), you should use:
  Type.stat_size_t = Type.off_t.withName("stat_size_t")

::: toolkit/components/osfile/modules/osfile_unix_back.jsm
@@ +616,4 @@
>           return result;
>         };
>  
> +       if (OS.Constants.Sys.Name == "NetBSD")

Nit: braces here, too.

Comment 8

[Martin Husemann]

Attachment: v4: create a special type for stat.st_size and deal with NetBSD libc symbol names

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 829188 [details] [diff] [review]
v4: create a special type for stat.st_size and deal with NetBSD libc symbol names

Review of attachment 829188 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thank you for the patch!

Comment 10

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f7246bf215

Comment 11

[Jan Beich]

(In reply to Martin Husemann from comment #0)
> It also fixes the "st_size" field in struct stat, which is not size_t but
> off_t in NetBSD.

st_size is of int64_t since 4.4 BSD and "long" before. At least on OS X 10.6 i386: off_t (long long) != size_t (unsigned long). And size check in worker_test_osfile_front.js doesn't seem to care.

How to test/confirm the issue with OS.File ?

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e5f7246bf215

Comment 13

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jan Beich from comment #11)
> st_size is of int64_t since 4.4 BSD and "long" before. At least on OS X 10.6
> i386: off_t (long long) != size_t (unsigned long). And size check in
> worker_test_osfile_front.js doesn't seem to care.
> 
> How to test/confirm the issue with OS.File ?

I just double-checked. Both Linux, NetBSD and MacOS X use off_t. We should probably switch everything to off_t.
Now, to see the difference, we would probably need to write a file of at least 2^31 + 1 bytes, which would require serious hacking.