lint: warn about #[no_mangle] fns that aren't exported #21495

richo · 2015-01-22T03:40:18Z

The usecase is that functions made visible to systems outside of the
rust ecosystem require the symbol to be visible.

This adds a lint for functions that are not exported, but also not mangled.

It has some gotchas:

[ ]: There is fallout in core that needs taking care of
[ ]: I'm not convinced the error message is correct
[ ]: It has no tests

However, there's an underlying issue which I'd like feedback on- which is that my belief that that non-pub functions would not have their symbols exported, however that seems not to be the case in the first case that this lint turned up in rustc (rust_fail), which intuition suggests has been working.

This seems to be a separate bug in rust, wherein the symbols are exported in binaries, but not in rlibs or dylibs. This lint would catch that case.

rust-highfive · 2015-01-22T03:40:32Z

r? @luqmana

(rust_highfive has picked a reviewer for you, use r? to override)

luqmana · 2015-01-25T07:17:42Z

I'm not sure if just checking for pub visibility is fine since it could be pub but in a non-pub mod. Also, no_mangle doesn't only go on fn's, it can apply to statics as well.

@alexcrichton any comments on whether we want to enforce no_mangle only on exported items?

richo · 2015-01-25T07:26:32Z

I can expand the lint to look at statics as well trivially, (And agree that's reasonable).

I can have a poke at trying to work out whether or not the symbol will actually end up exported at the crate level, but the mechanics of doing that aren't totally clear to me, eg I will probably need some help.

alexcrichton · 2015-01-25T07:30:31Z

I'm not sure if just checking for pub visibility is fine since it could be pub but in a non-pub mod

Yes this is where one of the maps calculated by rustc_privacy will come in handy. I believe the relevant one is called exported_items

I'm not convinced the error message is correct

The usage of :? seems surprising to me, it may want to use {} instead.

@alexcrichton any comments on whether we want to enforce no_mangle only on exported items?

I think this seems like a good lint to have as I can't think of a reason why you would want to #[no_mangle] a private function. The compiler will likely set its visibility such that it can't be reached anyway and at that point opting-out in a few rare locations seems like an ok idea.

alexcrichton · 2015-01-25T07:31:04Z

src/libstd/rt/unwind.rs

@@ -160,7 +160,7 @@ pub fn panicking() -> bool {
 // An uninlined, unmangled function upon which to slap yer breakpoints
 #[inline(never)]
 #[no_mangle]
-fn rust_panic(cause: Box<Any + Send>) -> ! {
+pub fn rust_panic(cause: Box<Any + Send>) -> ! {


This function should stay private, it's only used here to help debuggers.

This is why I'm so confused, it's not at all clear to me how this works, since all evidence suggests that this symbol should not be exported, nonetheless setting a breakpoint on it DTRT

The symbol is not available to link against but binaries typically still retain metadata about other symbols (even private ones) and in this case the metadata for rust_panic is kept around.

richo · 2015-01-26T04:36:36Z

I'm still working on this. It seems that if I want to use the machinery in rustc_privacy I end up with a circular dependency between rustc and rustc_privacy.

It seems like the obvious solution would be to pull the lints out into librustc_lint?

richo · 2015-01-28T06:41:21Z

So upon further digging I discovered that exported_items is actually exposed on lint::Context already, so the work to move it onto ty::ctxt was largely pointless.

(It is preserved for posterity reasons here: https://github.com/richo/rust/compare/refactor-ctxt-exported if this is actually something worth doing I'm happy to finish it up and migrate everything in lint to peek via ctxt)

r? @alexcrichton

alexcrichton · 2015-01-29T07:09:00Z

src/librustc/lint/builtin.rs

+                         it.span,
+                         format!("function {} is marked #[no_mangle], but not exported",
+                                 it.ident).as_slice());
+        }


This doesn't seem like it's too too large, could it be manually inlined below?

I did this because of rightward drift, attempting to avoid upsetting tidy. I'll have a poke.

alexcrichton · 2015-01-29T07:09:33Z

Looks good to me, thanks @richo! With a test I think that this is good to land.

richo · 2015-01-29T07:56:23Z

Now with a test! (Is this doing it right? It doesn't look like too many lints have tests, so I cargo culted something similar looking).

r? @alexcrichton

alexcrichton · 2015-01-29T23:24:53Z

src/librustc/lint/builtin.rs

@@ -2036,6 +2036,35 @@ impl LintPass for HardwiredLints {
    }
 }

+declare_lint! {
+    UNEXPORTED_NO_MANGLE,


In terms of a lint name, we may want to carefully consider our conventions in this area. This may not be the best name as allow(unexported_no_mangle) may not quite fit those conventions.

Some other possible names may be:

private-no-mangle-fns

priv-no-mangle-fns

private-unmangled-fns

Or some permutation of prefixes/suffixes. Our conventions say that because this only applies to functions it should probably have "fns" in the name somewhere though.

Thanks for the heads up. I didn't know that RFC existed. I have some mild
fud about priv purely because the keyword is gone but it might be confusing
to people who were around for that era (And I think they keyword is still
reserved, in which case this lint might end up being downright misleading).

As a strawman, how about non-pub-no-mangle-fns ? I realise that's a
mouthful, so I'd be totally ok with private-no-mangle-fns since it at
least doesn't conflict with a keyword. I tend to think that it should
include the name of the attr if at all possible to make grepping around for
things related to this do something reasonable.

On Thu, Jan 29, 2015 at 3:25 PM, Alex Crichton notifications@github.com
wrote:

In src/librustc/lint/builtin.rs
#21495 (comment):

@@ -2036,6 +2036,35 @@ impl LintPass for HardwiredLints {
}
}

+declare_lint! {

UNEXPORTED_NO_MANGLE,

In terms of a lint name, we may want to carefully consider our conventions
https://github.com/rust-lang/rfcs/blob/cc53afbe5dea41e1f7d1c3dce71e013abe025211/text/0344-conventions-galore.md
in this area. This may not be the best name as allow(unexported_no_mangle)
may not quite fit those conventions.

Some other possible names may be:

private-no-mangle-fns

priv-no-mangle-fns

private-unmangled-fns

Or some permutation of prefixes/suffixes. Our conventions say that because
this only applies to functions it should probably have "fns" in the name
somewhere though.

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/21495/files#r23812524.

Yeah out of this I think that private-no-mangle-fns is the best option. I agree with keeping no-mangle in the name and keeping priv out.

richo · 2015-01-29T23:52:53Z

I updated the PR to use the new name. I left the warning text alone (saying "But not exported" instead of mentioning private), specifically because it might not be obvious why this line fires, for example if you have a pub fn in an unexported module.

alexcrichton · 2015-01-29T23:54:00Z

@bors: r+ 75b5494

Thanks!

bors · 2015-01-30T05:05:13Z

⌛ Testing commit 75b5494 with merge e103563...

bors · 2015-01-30T05:26:27Z

💔 Test failed - auto-win-64-nopt-t

richo · 2015-01-30T05:29:11Z

oof, I didn't notice that they were platform specific. Fixing.

The usecase is that functions made visible to systems outside of the rust ecosystem require the symbol to be visible.

rust_panic is unexported, however the metadata exported will usually include it for attaching breakpoints.

richo · 2015-01-30T05:32:36Z

Ok, should be fixed. r? @alexcrichton

alexcrichton · 2015-01-30T05:44:18Z

@bors: r+ 1d13896

bors · 2015-01-30T10:34:52Z

⌛ Testing commit 1d13896 with merge a053886...

bors · 2015-01-30T10:52:35Z

💔 Test failed - auto-mac-64-nopt-t

richo · 2015-01-30T10:59:11Z

Sigh. My bad again. (On the upside, I caught that I had tagged a bunch of stuff that didn't need it by accident).

Sorry for all the noise with this one :( r? @alexcrichton

alexcrichton · 2015-01-30T17:09:31Z

@bors: r+ ff25fd6

no worries!

The usecase is that functions made visible to systems outside of the rust ecosystem require the symbol to be visible. This adds a lint for functions that are not exported, but also not mangled. It has some gotchas: [ ]: There is fallout in core that needs taking care of [ ]: I'm not convinced the error message is correct [ ]: It has no tests ~~However, there's an underlying issue which I'd like feedback on- which is that my belief that that non-pub functions would not have their symbols exported, however that seems not to be the case in the first case that this lint turned up in rustc (`rust_fail`), which intuition suggests has been working.~~ This seems to be a separate bug in rust, wherein the symbols are exported in binaries, but not in rlibs or dylibs. This lint would catch that case.

bors · 2015-01-30T19:56:35Z

⌛ Testing commit ff25fd6 with merge 1d00c54...

The usecase is that functions made visible to systems outside of the rust ecosystem require the symbol to be visible. This adds a lint for functions that are not exported, but also not mangled. It has some gotchas: [ ]: There is fallout in core that needs taking care of [ ]: I'm not convinced the error message is correct [ ]: It has no tests ~~However, there's an underlying issue which I'd like feedback on- which is that my belief that that non-pub functions would not have their symbols exported, however that seems not to be the case in the first case that this lint turned up in rustc (`rust_fail`), which intuition suggests has been working.~~ This seems to be a separate bug in rust, wherein the symbols are exported in binaries, but not in rlibs or dylibs. This lint would catch that case.

bors · 2015-01-30T23:07:58Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

rust-highfive assigned luqmana Jan 22, 2015

alexcrichton reviewed Jan 25, 2015
View reviewed changes

richo force-pushed the unexported-unmangled-lint branch 2 times, most recently from a043108 to 5731706 Compare January 28, 2015 06:25

rust-highfive assigned alexcrichton and unassigned luqmana Jan 28, 2015

alexcrichton reviewed Jan 29, 2015
View reviewed changes

richo force-pushed the unexported-unmangled-lint branch 2 times, most recently from b473278 to 556951e Compare January 29, 2015 07:55

alexcrichton reviewed Jan 29, 2015
View reviewed changes

richo force-pushed the unexported-unmangled-lint branch from 556951e to 75b5494 Compare January 29, 2015 23:51

richo added 2 commits January 29, 2015 21:32

lint: warn about #[no_mangle] fns that aren't exported

44ff721

The usecase is that functions made visible to systems outside of the rust ecosystem require the symbol to be visible.

rt: allow unexported functions

d2ab7d3

rust_panic is unexported, however the metadata exported will usually include it for attaching breakpoints.

richo force-pushed the unexported-unmangled-lint branch from 75b5494 to 1d13896 Compare January 30, 2015 05:32

lint: Add test for no_mangle

ff25fd6

richo force-pushed the unexported-unmangled-lint branch from 1d13896 to ff25fd6 Compare January 30, 2015 10:58

bors merged commit ff25fd6 into rust-lang:master Jan 30, 2015

lint: warn about #[no_mangle] fns that aren't exported #21495

lint: warn about #[no_mangle] fns that aren't exported #21495

Uh oh!

Conversation

richo commented Jan 22, 2015

Uh oh!

rust-highfive commented Jan 22, 2015

Uh oh!

luqmana commented Jan 25, 2015

Uh oh!

richo commented Jan 25, 2015

Uh oh!

alexcrichton commented Jan 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richo commented Jan 26, 2015

Uh oh!

richo commented Jan 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Jan 29, 2015

Uh oh!

richo commented Jan 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richo commented Jan 29, 2015

Uh oh!

alexcrichton commented Jan 29, 2015

Uh oh!

bors commented Jan 30, 2015

Uh oh!

bors commented Jan 30, 2015

Uh oh!

richo commented Jan 30, 2015

Uh oh!

richo commented Jan 30, 2015

Uh oh!

alexcrichton commented Jan 30, 2015

Uh oh!

bors commented Jan 30, 2015

Uh oh!

bors commented Jan 30, 2015

Uh oh!

richo commented Jan 30, 2015

Uh oh!

alexcrichton commented Jan 30, 2015

Uh oh!

bors commented Jan 30, 2015

Uh oh!

bors commented Jan 30, 2015

Uh oh!

Uh oh!