1459314 - NSS is missing every(?) public symbol when -fvisibility=hidden is specified

Status: NEW

(Firefox Build System :: General, enhancement, P3)

Description

[Tom Ritter [:tjr] (OOTO until 9/2)]

Whole bunch of errors of the form 

> 0:07.58 /builds/worker/my-workspace/build/src/clang/bin/ld.lld: error: undefined symbol: NSS_Initialize
> 0:07.58 >>> referenced by mar_sign.c:37 (/builds/worker/my-workspace/build/src/modules/libmar/sign/mar_sign.c:37)
> 0:07.58 >>>               lto.tmp:(main)
> 0:07.58 /builds/worker/my-workspace/build/src/clang/bin/ld.lld: error: undefined symbol: PK11_FindCertFromNickname
> 0:07.58 >>> referenced by mar.c:365 (/builds/worker/my-workspace/build/src/modules/libmar/tool/mar.c:365)
> 0:07.58 >>>               lto.tmp:(main)


I compared a LTO build with a LTO+CFI build, taking NSS_Initialize as an arbitrary example. Sure enough, it's missing in the CFI build: https://www.diffchecker.com/QhA60FqE

Comment 1

[Tom Ritter [:tjr] (OOTO until 9/2)]

This does not occur with -fsanitize=cfi-cast-strict but it does occur with -fsanitize=cfi-icall. Neither of these require -fvisibility, so the error apparently does not related to that flag.

Comment 2

[(Away)]

Are the undefined symbols always (for lack of knowing the elf term) "dllexport" functions?

Comment 3

[Tom Ritter [:tjr] (OOTO until 9/2)]

No longer a problem. Maybe upgrading clang solved it?

Comment 4

[Tom Ritter [:tjr] (OOTO until 9/2)]

Nope, spoke to soon. This error comes from cfi-vcall

Example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=260914a4b1b2cc32050fbff8fe9b927e43537165&selectedJob=182636347

Comment 5

[Tom Ritter [:tjr] (OOTO until 9/2)]

This is because the functions that don't get emitted to the DSO need an __attribute__((visibility("default"))) annotation

Comment 6

[Mike Hommey [:glandium]]

(In reply to Tom Ritter [:tjr] from comment #5)
> This is because the functions that don't get emitted to the DSO need an
> __attribute__((visibility("default"))) annotation

That doesn't sound right.

Comment 7

[Tom Ritter [:tjr] (OOTO until 9/2)]

(In reply to Mike Hommey [:glandium] from comment #6)
> (In reply to Tom Ritter [:tjr] from comment #5)
> > This is because the functions that don't get emitted to the DSO need an
> > __attribute__((visibility("default"))) annotation
> 
> That doesn't sound right.

Ah... I didn't mention this; but this error occurs when adding -fvisibility=hidden to the build and linking command line tools to nss3 like signmar.  Does that sound any better?

Comment 8

[Mike Hommey [:glandium]]

Why are you adding -fvisibility=hidden to the build?

Comment 9

[Tom Ritter [:tjr] (OOTO until 9/2)]

(In reply to Mike Hommey [:glandium] from comment #8)
> Why are you adding -fvisibility=hidden to the build?

Some value of -fvisibility is required for cfi-vcall.  fvisibility=hidden is the generally recommended option. https://clang.llvm.org/docs/ControlFlowIntegrity.html

Comment 10

[Mike Hommey [:glandium]]

Except we set visibility in other ways. Which is why adding -fvisibility=hidden busts NSS. And does nothing useful on the rest on the build.

Comment 11

[Tom Ritter [:tjr] (OOTO until 9/2)]

Attachment: Bug 1459314 Add default visibility to a whole bunch of stuff in nss and sqlite to resolve missing symbol errors with cfi-vcall

Review commit: https://reviewboard.mozilla.org/r/260054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260054/

Comment 12

[Tom Ritter [:tjr] (OOTO until 9/2)]

Regardless, fvisibility is a required compiler flag for CFI vcall to compile.

I've attached a horrible patch that adds the attribute to the public NSS functions, and allows a browser with vcall to compile and run.  We may need to add the appropriate attribute (as indicated in this patch); but another option may include setting -fvisibility=default for NSS (although I think this will result in no CFI protection there).

Comment 13

[Tom Ritter [:tjr] (OOTO until 9/2)]

Attachment: Bug 1459314 Add default visibility to a whole bunch of stuff in nss and sqlite to resolve missing symbol errors with cfi-vcall r=Alex_Gaynor

Comment 14

[Mike Hommey [:glandium]]

(In reply to Tom Ritter [:tjr] from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > Why are you adding -fvisibility=hidden to the build?
> 
> Some value of -fvisibility is required for cfi-vcall.  fvisibility=hidden is
> the generally recommended option.
> https://clang.llvm.org/docs/ControlFlowIntegrity.html

I'll have to insist that you don't use -fvisibility=hidden. What the page says is that the build needs to have visibility hidden associated with non public stuff so that CFI can work. That already *is* happening with the normal build. You don't need to pass -fvisibility=hidden to have symbols with hidden visibility in the build.

Comment 15

[Tom Ritter [:tjr] (OOTO until 9/2)]

(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to Tom Ritter [:tjr] from comment #9)
> > (In reply to Mike Hommey [:glandium] from comment #8)
> > > Why are you adding -fvisibility=hidden to the build?
> > 
> > Some value of -fvisibility is required for cfi-vcall.  fvisibility=hidden is
> > the generally recommended option.
> > https://clang.llvm.org/docs/ControlFlowIntegrity.html
> 
> I'll have to insist that you don't use -fvisibility=hidden. What the page
> says is that the build needs to have visibility hidden associated with non
> public stuff so that CFI can work. That already *is* happening with the
> normal build. You don't need to pass -fvisibility=hidden to have symbols
> with hidden visibility in the build.

I will have to figure out what I need to change in the compiler to get it to accept vcall without specifying the compiler parameter.

Right now; the direct milestone for CFI is to get performance numbers on Windows acceptable; so I'll be using this local workaround for the time being until we know for certain we can land CFI at all; at which point cleaning up the ugly messes like this one will be the priority (and blockers for landing).

Comment 16

[Mike Hommey [:glandium]]

That the compiler requires -fvisibility=hidden is even worse on Windows, where it has no meaning: everything is hidden by default, unless __declspec(dllexport).

Comment 17

[Tom Ritter [:tjr] (OOTO until 9/2)]

Attachment: Bug 1459314 Add default visibility to a whole bunch of stuff in nss and sqlite to resolve missing symbol errors with cfi-vcall r=Alex_Gaynor

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The following patches are waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D3041	Bug 1459314 Add default visibility to a whole bunch of stuff in nss and sqlite to resolve missing symbol errors with cfi-vcall r=Alex_Gaynor	tjr	Alex_Gaynor: Inactive
D3419	Bug 1459314 Add default visibility to a whole bunch of stuff in nss and sqlite to resolve missing symbol errors with cfi-vcall r=Alex_Gaynor	tjr	Alex_Gaynor: Inactive

:tjr, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.