Stream: git-wasmtime

Topic: wasmtime / PR #1455 cranelift-object: mark output as usin...

Wasmtime GitHub notifications bot (Apr 01 2020 at 15:16):

froydnj opened PR #1455 from object-noexecstack to master:

The faerie crate does this by default. object itself does not, but
it seems reasonable to make this change for parity between
cranelift-faerie and cranelift-object.

Wasmtime GitHub notifications bot (Apr 01 2020 at 15:53):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Apr 01 2020 at 15:53):

bjorn3 created PR Review Comment:

Should this only be added for GNU systems or is it fine to just keep it on other systems without purpose?

Wasmtime GitHub notifications bot (Apr 01 2020 at 15:53):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Apr 01 2020 at 16:08):

froydnj submitted PR Review.

Wasmtime GitHub notifications bot (Apr 01 2020 at 16:08):

froydnj created PR Review Comment:

I read this as "should we only add this for *gnu targets (e.g. not FreeBSD)" or "should we unconditionally add it everywhere (e.g. Darwin, Windows)", is that correct? I don't think we should add it everywhere, so that leaves just the first question.

Browsing through assembly files in Firefox (Firefox code itself and various third-party libraries), it looks like people are inconsistent in their choices: some restrict this note to Linux-only and some restrict it to ELF-only. I think making the condition target_lexicon::OperatingSystem::Linux would be fine-ish (I don't know what the *BSDs do with respect to this section; I see that lld ignores this section entirely and controls executable stack-ness with a command-line switch). I don't think I'd want to try to condition on *gnu-ness; matching on target_lexicon::Environment to catch all of the cases would be annoying, I think -- and maybe you also have to catch musl-ness etc. as well?

Making it ELF-only seems like the safest option, and the linker should just drop these sections anyway.

Wasmtime GitHub notifications bot (Apr 01 2020 at 16:19):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Apr 01 2020 at 16:19):

bjorn3 created PR Review Comment:

I read this as "should we only add this for *gnu targets (e.g. not FreeBSD)" or "should we unconditionally add it everywhere (e.g. Darwin, Windows)", is that correct?

Almost. I meant should this be added for all ELF systems, or only GNU based ones.

I think making the condition target_lexicon::OperatingSystem::Linux would be fine-ish

There are several non-Linux systems which use a GNU userspace, like Hurd and (as option?) Plan 9.

I see that lld ignores this section entirely and controls executable stack-ness with a command-line switch

:tada: .note.GNU-stack makes it way too easy to accidentally end up with an executable stack.

Making it ELF-only seems like the safest option, and the linker should just drop these sections anyway.

:+1:

Wasmtime GitHub notifications bot (Apr 01 2020 at 16:20):

froydnj merged PR #1455.

Last updated: Oct 23 2024 at 20:03 UTC