1449835 - nsTestCrasher.cpp does not compile for MinGW x64

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tom Ritter [:tjr]]

nsTestCrasher.cpp uses an assembly file that doesn't compile for MinGW x64.

Comment 1

[Tom Ritter [:tjr]]

Attachment: Bug 1449835 Do not compile Windows x64 Crash Test Assembly for MinGW

The assembly file uses the wrong syntax and MinGW cannot compile it.

(Also, gcc doesn't recognize it, because it ends in .asm and not .s.)

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

Comment 2

[Tom Ritter [:tjr]]

Comment on attachment 8963462 [details]
Bug 1449835 Do not compile Windows x64 Crash Test Assembly for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232386/diff/1-2/

Comment 3

[Carl Corcoran [:ccorcoran]]

Can we find a way to make this work on MinGW instead of just disabling the feature?

Comment 4

[Tom Ritter [:tjr]]

So, as I understand it, this code is testing that the Call Frame Information is correct in different crash scenarios for breakpad? I don't think this is useful for MinGW at all - we don't ship this to users so we're not getting stacks from them; I don't know if the CFI fixup is needed on MinGW; MinGW generates DWARF info, not pdbs; we don't have any symbols on MinGW at all right now because the DWARF debug info it generates is (we think) bigger than Windows dll size limit.

In theory I suppose one could port the assembly to a syntax gcc would understand; but I don't know how to do that right now and it doesn't seem like a good investment of time.

Comment 5

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8963462 [details]
Bug 1449835 Do not compile Windows x64 Crash Test Assembly for MinGW

https://reviewboard.mozilla.org/r/232384/#review240552

Sorry for the delay but this seems fine to me!

Comment 6

[Carl Corcoran [:ccorcoran]]

Comment on attachment 8963462 [details]
Bug 1449835 Do not compile Windows x64 Crash Test Assembly for MinGW

https://reviewboard.mozilla.org/r/232386/#review238746

::: commit-message-c9306:3
(Diff revision 2)
> +Bug 1449835 Do not compile Windows x64 Crash Test Assembly for MinGW r?ccorcoran
> +
> +The assembly file uses the wrong syntax and MinGW cannot compile it.

Would it be possible instead to adapt the code to build properly under mingw?

Comment 7

[Carl Corcoran [:ccorcoran]]

Sorry for that last comment... still getting used to reviewboard :x

Comment 8

[Cristian Brindusan [:cbrindusan]]

Hi,

When I was trying to land this patch, I've encountered the following error:

hg import https://reviewboard-hg.mozilla.org/gecko/rev/2c202e2490dba8223fecd7268e978a5cb614dae4

applying https://reviewboard-hg.mozilla.org/gecko/rev/2c202e2490dba8223fecd7268e978a5cb614dae4

patching file toolkit/crashreporter/test/nsTestCrasher.cpp

Hunk #4 FAILED at 112
Hunk #6 FAILED at 167
2 out of 7 hunks FAILED -- saving rejects to file toolkit/crashreporter/test/nsTestCrasher.cpp.rej
abort: patch failed to apply

Can you please take a look?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

MozReview also says it doesn't have the proper reviews needed for Autoland to push it.

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 8963462 [details]
Bug 1449835 Do not compile Windows x64 Crash Test Assembly for MinGW

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232386/diff/2-3/

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8963462 [details]
Bug 1449835 Do not compile Windows x64 Crash Test Assembly for MinGW

https://reviewboard.mozilla.org/r/232386/#review241972

r=me for the moz.build bits.

Comment 12

[Tom Ritter [:tjr]]

Okay, rebased, and should be landable.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c498706ccf3
Do not compile Windows x64 Crash Test Assembly for MinGW r=ccorcoran,froydnj

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3c498706ccf3

Comment 15

[Tom Ritter [:tjr]]

Attachment: esr60.patch

[Approval Request Comment]

This is one of several MinGW Build patches I'd like to land in esr60 for Tor. It will prevent them from carrying their own patches for the lifetime of esr60 and will enable us to keep the MinGW build functioning and know if/when/how it was broken by new commits into esr60.

This commit only affects the MinGW build configuration, so it is low-risk.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8976255 [details] [diff] [review]
esr60.patch

mingw build fix, approved for 60.1esr

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/5dc9b3739510