Closed
Bug 1235132
Opened 8 years ago
Closed 8 years ago
Add support for a more-or-less cross-platform symbols file
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox46 fixed)
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(5 files, 2 obsolete files)
4.88 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
15.21 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
8.33 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
12.25 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
gps
:
review+
benjamin
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8702001 -
Flags: review?(gps)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8702002 -
Flags: review?(gps)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8702003 -
Flags: review?(gps)
Assignee | ||
Comment 4•8 years ago
|
||
Rewrote so that the sqlite.def -> sqlite.symbols diff is simpler to review.
Attachment #8702002 -
Attachment is obsolete: true
Attachment #8702002 -
Flags: review?(gps)
Attachment #8702004 -
Flags: review?(gps)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8702005 -
Flags: review?(gps)
Assignee | ||
Updated•8 years ago
|
Attachment #8702003 -
Attachment description: Remove convert_def_file.py → Part 4 - Remove convert_def_file.py
Assignee | ||
Updated•8 years ago
|
Attachment #8702004 -
Attachment description: Convert sqlite and nss to SYMBOLS_FILE → Part 2 - Convert sqlite and nss to SYMBOLS_FILE
Assignee | ||
Updated•8 years ago
|
Attachment #8702005 -
Attachment description: Remove whitespaces from sqlite.symbols → Part 3 - Remove whitespaces from sqlite.symbols
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8702001 -
Attachment is obsolete: true
Attachment #8702001 -
Flags: review?(gps)
Attachment #8702006 -
Flags: review?(gps)
Comment 7•8 years ago
|
||
I now see warnings such as " 1:08.00 ld: warning: cannot export hidden symbol __PR_Darwin_x86_64_AtomicIncrement from ../../../nsprpub/pr/src/md/unix/os_Darwin.o 1:08.00 ld: warning: cannot export hidden symbol __PR_Darwin_x86_64_AtomicDecrement from ../../../nsprpub/pr/src/md/unix/os_Darwin.o 1:08.00 ld: warning: cannot export hidden symbol __PR_Darwin_x86_64_AtomicSet from ../../../nsprpub/pr/src/md/unix/os_Darwin.o 1:08.00 ld: warning: cannot export hidden symbol __PR_Darwin_x86_64_AtomicAdd from ../../../nsprpub/pr/src/md/unix/os_Darwin.o "
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Jean-Yves Avenard [:jya] from comment #7) > I now see warnings such as " 1:08.00 ld: warning: cannot export hidden > symbol __PR_Darwin_x86_64_AtomicIncrement from > ../../../nsprpub/pr/src/md/unix/os_Darwin.o > 1:08.00 ld: warning: cannot export hidden symbol > __PR_Darwin_x86_64_AtomicDecrement from > ../../../nsprpub/pr/src/md/unix/os_Darwin.o > 1:08.00 ld: warning: cannot export hidden symbol > __PR_Darwin_x86_64_AtomicSet from ../../../nsprpub/pr/src/md/unix/os_Darwin.o > 1:08.00 ld: warning: cannot export hidden symbol > __PR_Darwin_x86_64_AtomicAdd from ../../../nsprpub/pr/src/md/unix/os_Darwin.o > " Not a problem.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #8) > (In reply to Jean-Yves Avenard [:jya] from comment #7) > > I now see warnings such as " 1:08.00 ld: warning: cannot export hidden > > symbol __PR_Darwin_x86_64_AtomicIncrement from > > ../../../nsprpub/pr/src/md/unix/os_Darwin.o > > 1:08.00 ld: warning: cannot export hidden symbol > > __PR_Darwin_x86_64_AtomicDecrement from > > ../../../nsprpub/pr/src/md/unix/os_Darwin.o > > 1:08.00 ld: warning: cannot export hidden symbol > > __PR_Darwin_x86_64_AtomicSet from ../../../nsprpub/pr/src/md/unix/os_Darwin.o > > 1:08.00 ld: warning: cannot export hidden symbol > > __PR_Darwin_x86_64_AtomicAdd from ../../../nsprpub/pr/src/md/unix/os_Darwin.o > > " > > Not a problem. All that actually says is that we can probably remove the "_PR_*" entry from the symbols file without any problem.
Assignee | ||
Comment 10•8 years ago
|
||
Didn't feel like filing a separate bug for this.
Attachment #8702460 -
Flags: review?(gps)
Comment 11•8 years ago
|
||
Comment on attachment 8702006 [details] [diff] [review] Part 1 - Add support for a more-or-less cross-platform symbols file Review of attachment 8702006 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +461,5 @@ > EXTRA_DEPS += $(LD_VERSION_SCRIPT) > endif > endif > > +ifdef SYMBOLS_FILE I'm not the biggest fan of adding this to rules.mk - I'd prefer writing it to backend.mk from recursivemake.py. But I'll allow it.
Attachment #8702006 -
Flags: review?(gps) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8702004 [details] [diff] [review] Part 2 - Convert sqlite and nss to SYMBOLS_FILE Review of attachment 8702004 [details] [diff] [review]: ----------------------------------------------------------------- Yay for eliminating gnarly make foo.
Attachment #8702004 -
Flags: review?(gps) → review+
Updated•8 years ago
|
Attachment #8702005 -
Flags: review?(gps) → review+
Updated•8 years ago
|
Attachment #8702003 -
Flags: review?(gps) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8702460 [details] [diff] [review] Remove _PR_* symbols from nss.symbols Review of attachment 8702460 [details] [diff] [review]: ----------------------------------------------------------------- Not sure if I fully grok the implications of this since I'm not intimate with the C++ and 3rd party linking side of things. But not exporting "internal" symbols sounds reasonable to me. One concern I have is for plugin and 3rd party app compatibility. I reckon if there is fallout we can revert this easily. Flagging bsmedberg in case he has an opinion.
Attachment #8702460 -
Flags: review?(gps)
Attachment #8702460 -
Flags: review+
Attachment #8702460 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 14•8 years ago
|
||
Note the only affected symbols are _PR_<architecture>_Atomic{Decrement,Set,Add,Increment}, they are not exposed in public headers, have a different name on each architecture, and have a public API: PR_Atomic{Decrement,Set,Add,Increment}.
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b61f9f09b04 https://hg.mozilla.org/integration/mozilla-inbound/rev/186450f22aab https://hg.mozilla.org/integration/mozilla-inbound/rev/3b12a8b128ac https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee36e6d8dc7 https://hg.mozilla.org/integration/mozilla-inbound/rev/5db0dc925186
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b61f9f09b04 https://hg.mozilla.org/mozilla-central/rev/186450f22aab https://hg.mozilla.org/mozilla-central/rev/3b12a8b128ac https://hg.mozilla.org/mozilla-central/rev/8ee36e6d8dc7 https://hg.mozilla.org/mozilla-central/rev/5db0dc925186
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 17•8 years ago
|
||
Comment on attachment 8702460 [details] [diff] [review] Remove _PR_* symbols from nss.symbols If this doesn't break our build, it should be fine.
Attachment #8702460 -
Flags: feedback?(benjamin) → feedback+
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•