Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OpenSSL windows asm question #7426

Closed
indutny opened this issue Jun 26, 2016 · 7 comments
Closed

OpenSSL windows asm question #7426

indutny opened this issue Jun 26, 2016 · 7 comments
Labels
openssl Issues and PRs related to the OpenSSL dependency. windows Issues and PRs related to the Windows platform.

Comments

@indutny
Copy link
Member

indutny commented Jun 26, 2016

Part 1

01fa5ee

What is the purpose of this? cc @piscisaureus

As far as I get it - GYP option may be used to enable this: https://github.com/svn2github/gyp/blob/e0ee72ddc7fb97eb33d530cf684efcbe4d27ecb3/test/win/ml-safeseh/ml-safeseh.gyp#L18-L20

Part 2

What is the purpose of /Zi? I know that it generates debug info, but can't see why it is needed. cc @piscisaureus again

cc @nodejs/collaborators just in case.

Reasoning

I'm thinking about removing rules from the gyp.js as I don't really like the feature bloat that it has. It looks like Node builds just fine without these rules
@indutny
Copy link
Member Author

indutny commented Jun 26, 2016

For part 2 DebugInformationFormat option may be used as well.

@indutny indutny mentioned this issue Jun 26, 2016
Open
@indutny
Copy link
Member Author

indutny commented Jun 26, 2016

Also, .asm files are supported by GYP since I don't know what time, so there is no reason to have extra rules for them.

@mscdex mscdex added windows Issues and PRs related to the Windows platform. openssl Issues and PRs related to the OpenSSL dependency. labels Jun 26, 2016
indutny added a commit that referenced this issue Jun 26, 2016
@indutny
deps: MASM.UseSafeExceptionHandlers for OpenSSL
eb57503 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
@indutny indutny mentioned this issue Jun 26, 2016
Closed
4 tasks
@indutny
Copy link
Member Author

indutny commented Jun 26, 2016

Proposed change: #7427

@bnoordhuis
Copy link
Member

01fa5ee

Not a Windows compiler/linker expert but I suspect that without /safeseh you wouldn't be able to link add-ons that depend on SafeSEH.

@eljefedelrodeodeljefe
Copy link
Contributor

+1 for removing /Zi, also for all that .pdb files and manifests and some assembly inlining. For some of those flags even the MSDN pages warn for extreme file bloat.

/SafeSEH: I can only guess, but I assume it predates the current compilation workflow, where openssl has been compiled with an older compiler, that didn't include /safeseh and we wanted to protect against it. The MSDN page contradicts the commit message, as the flag does not mark afile, but protect against it at compile time. I found this to be useful, when I decided to exclude it. However it should not be expensive to let it in.

@piscisaureus
Copy link
Contributor

@indutny, @eljefedelrodeodeljefe: for /safeseh: see nodejs/node-v0.x-archive#4242

I think you can drop /Zi.

@indutny
Copy link
Member Author

indutny commented Jun 27, 2016

@piscisaureus thank you, asked you a question in #7427 ;)

Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
@indutny @Fishrock123
deps: MASM.UseSafeExceptionHandlers for OpenSSL
a4880b5 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
indutny added a commit to indutny/io.js that referenced this issue Sep 3, 2016
@indutny
deps: MASM.UseSafeExceptionHandlers for OpenSSL
72a08e2 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: nodejs#7426
PR-URL: nodejs#7427
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 4, 2016
@indutny @MylesBorins
deps: MASM.UseSafeExceptionHandlers for OpenSSL
ef977f6 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 28, 2016
@indutny
deps: MASM.UseSafeExceptionHandlers for OpenSSL
77827eb 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
rvagg pushed a commit that referenced this issue Oct 18, 2016
@indutny @rvagg
deps: MASM.UseSafeExceptionHandlers for OpenSSL
0e1162c 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 26, 2016
@indutny @MylesBorins
deps: MASM.UseSafeExceptionHandlers for OpenSSL
ea36c61 
Use `msvs_settings.MASM.UseSafeExceptionHandlers` when building OpenSSL
assembly code on Windows. This option appends `/safeseh` to the list of
assembler flags when building `.asm` files on Windows.

Having this option in place, separate rules in `masm_compile.gypi` are
no longer needed.

Fix: #7426
PR-URL: #7427
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Bert Belder <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
openssl Issues and PRs related to the OpenSSL dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

deps: MASM.UseSafeExceptionHandlers for OpenSSL indutny/io.js
5 participants
@mscdex @piscisaureus @indutny @bnoordhuis @eljefedelrodeodeljefe