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

RESOLVED FIXED in mozilla28

Status

()

Toolkit
OS.File
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Martin Husemann, Assigned: Martin Husemann)

Tracking

Trunk
mozilla28
All
NetBSD
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 828625 [details] [diff] [review]
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.
Attachment #828625 - Flags: review?(dteller)
(Assignee)

Comment 1

4 years ago
Created attachment 828628 [details] [diff] [review]
Ryo ONODERA points out another st_size hardcoded type which needs to be off_t on NetBSD
(Assignee)

Updated

4 years ago
Attachment #828628 - Flags: review?(dteller)
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 {
  ...
}
Attachment #828625 - Flags: review?(dteller) → review+
Summary: Firefox does not restore tabs on NetBSD → [OS.File] Firefox does not restore tabs on NetBSD
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.
Attachment #828628 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 4

4 years ago
(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" ?
(Assignee)

Comment 5

4 years ago
Created attachment 829163 [details] [diff] [review]
v2: create a special type for stat.st_size and deal with NetBSD libc symbol names
Attachment #828625 - Attachment is obsolete: true
Attachment #828628 - Attachment is obsolete: true
Attachment #829163 - Flags: review?(dteller)
(Assignee)

Comment 6

4 years ago
Created attachment 829165 [details] [diff] [review]
v3: create a special type for stat.st_size and deal with NetBSD libc symbol names

oops, missed a few { } previously
Attachment #829163 - Attachment is obsolete: true
Attachment #829163 - Flags: review?(dteller)
Attachment #829165 - Flags: review?(dteller)
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.
Attachment #829163 - Attachment is obsolete: false
(Assignee)

Comment 8

4 years ago
Created attachment 829188 [details] [diff] [review]
v4: create a special type for stat.st_size and deal with NetBSD libc symbol names
Attachment #829163 - Attachment is obsolete: true
Attachment #829165 - Attachment is obsolete: true
Attachment #829165 - Flags: review?(dteller)
Attachment #829188 - Flags: review?(dteller)
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!
Attachment #829188 - Flags: review?(dteller) → review+

Comment 11

4 years ago
(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 ?
Flags: needinfo?(dteller)
https://hg.mozilla.org/mozilla-central/rev/e5f7246bf215
Assignee: nobody → martin
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(dteller)

Updated

4 years ago
Depends on: 939527
You need to log in before you can comment on or make changes to this bug.