Closed Bug 1235132 Opened 4 years ago Closed 4 years ago

Add support for a more-or-less cross-platform symbols file

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(5 files, 2 obsolete files)

No description provided.
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)
Attachment #8702003 - Attachment description: Remove convert_def_file.py → Part 4 - Remove convert_def_file.py
Attachment #8702004 - Attachment description: Convert sqlite and nss to SYMBOLS_FILE → Part 2 - Convert sqlite and nss to SYMBOLS_FILE
Attachment #8702005 - Attachment description: Remove whitespaces from sqlite.symbols → Part 3 - Remove whitespaces from sqlite.symbols
Blocks: 1214462
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
"
(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.
(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.
Didn't feel like filing a separate bug for this.
Attachment #8702460 - Flags: review?(gps)
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 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+
Attachment #8702005 - Flags: review?(gps) → review+
Attachment #8702003 - Flags: review?(gps) → review+
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)
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}.
Depends on: 1237140
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+
Depends on: 1239672
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.