[Linux] Turn off -mstackrealign for breakpad in ia32.

[Linux] Turn off -mstackrealign for breakpad in ia32.

There's a clang bug which causes crashes of syscalls which use many
registers for argument passing in ia32 architecture.
https://llvm.org/bugs/show_bug.cgi?id=16830

BUG=556393
Committed: https://crrev.com/176ff2c7a3285a851a534603e2e893664ec5ace7
Cr-Commit-Position: refs/heads/master@{#365770}

[Lukasz Jagielski, 2015-11-24 11:11:07]

Description was changed from

==========
[Linux] Turn off -mstackrealign for breakpad in ia32.

There's a clang bug which causes crashes of syscalls which use many
registers for argument passing in ia32 architecture.
https://llvm.org/bugs/show_bug.cgi?id=16830

BUG=556393
==========

to

==========
[Linux] Turn off -mstackrealign for breakpad in ia32.

There's a clang bug which causes crashes of syscalls which use many
registers for argument passing in ia32 architecture.
https://llvm.org/bugs/show_bug.cgi?id=16830

BUG=556393
==========

[Lukasz Jagielski, 2015-11-24 11:11:07]

ljagielski@opera.com changed reviewers:
+ thestig@chromium.org

[Lukasz Jagielski, 2015-11-24 11:11:27]

Hi,
PTAL at my CL proposition.
Regards,
Łukasz

[Lei Zhang, 2015-11-24 15:03:20]

Have you looked at components/crash/content/app/breakpad_linux.cc to see if it's
ok? Does it contain any syscalls which require at least 4 parameters?

https://codereview.chromium.org/1473203002/diff/1/breakpad/breakpad.gyp
File breakpad/breakpad.gyp (right):

https://codereview.chromium.org/1473203002/diff/1/breakpad/breakpad.gyp#newco...
breakpad/breakpad.gyp:600: # Clang's -mstackrealign  doesn't work well with
nits (ditto in all the other comments):

- Just 1 space after -mstackrealign.
- Can you reference https://crbug.com/556393 ?

[Lukasz Jagielski, 2015-11-24 16:10:11]

On 2015/11/24 15:03:20, Lei Zhang wrote:
> Have you looked at components/crash/content/app/breakpad_linux.cc to see if
it's
> ok? Does it contain any syscalls which require at least 4 parameters?
> 
> https://codereview.chromium.org/1473203002/diff/1/breakpad/breakpad.gyp
> File breakpad/breakpad.gyp (right):
> 
>
https://codereview.chromium.org/1473203002/diff/1/breakpad/breakpad.gyp#newco...
> breakpad/breakpad.gyp:600: # Clang's -mstackrealign  doesn't work well with
> nits (ditto in all the other comments):
> 
> - Just 1 space after -mstackrealign.
> - Can you reference https://crbug.com/556393 ?

Hi,
I've just took a look at components/crash/app/breakpad_linux.cc and there's a
sys_socketpair call which requires 4 arguments. Can I verify it somehow, if this
call is problematic? It looks to me that it's invoked on non-browser process
crash, should every chromium build try to upload crash dump somewhere? I added
the
line BUG=556393 to Description, is this what you meant by referencing the bug?
Łukasz

[Lei Zhang, 2015-11-24 18:17:29]

On 2015/11/24 16:10:11, Lukasz Jagielski wrote:
> Hi,
> I've just took a look at components/crash/app/breakpad_linux.cc and there's a
> sys_socketpair call which requires 4 arguments. Can I verify it somehow, if
this
> call is problematic? It looks to me that it's invoked on non-browser process
> crash, should every chromium build try to upload crash dump somewhere? I added
> the
> line BUG=556393 to Description, is this what you meant by referencing the bug?
> Łukasz

If you run with CHROME_HEADLESS=1, I think Chromium should try to generate a
dump. I'm just eyeballing it, but you should be able to hit this path by simply
visiting about:crash.

I meant to add comments in the code to reference the bug as well. Over time,
code gets moved around and git blame is not perfect, so the context for why
something gets added may get lost.

[Lukasz Jagielski, 2015-11-27 16:53:05]

Hi,
Thanks for the tip, changes in components/crash are needed. Crash dumping works
now.
Regards,
Łukasz

[Lei Zhang, 2015-12-17 04:14:42]

lgtm

Sorry for losing track of this CL.

[Lei Zhang, 2015-12-17 04:15:57]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-12-17 04:16:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473203002/40001

[commit-bot: I haz the power, 2015-12-17 04:53:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 04:53:28]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lei Zhang, 2015-12-17 06:23:26]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-12-17 06:23:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473203002/40001

[commit-bot: I haz the power, 2015-12-17 07:04:35]

Description was changed from

==========
[Linux] Turn off -mstackrealign for breakpad in ia32.

There's a clang bug which causes crashes of syscalls which use many
registers for argument passing in ia32 architecture.
https://llvm.org/bugs/show_bug.cgi?id=16830

BUG=556393
==========

to

==========
[Linux] Turn off -mstackrealign for breakpad in ia32.

There's a clang bug which causes crashes of syscalls which use many
registers for argument passing in ia32 architecture.
https://llvm.org/bugs/show_bug.cgi?id=16830

BUG=556393
==========

[commit-bot: I haz the power, 2015-12-17 07:04:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-17 07:05:12]

Description was changed from

==========
[Linux] Turn off -mstackrealign for breakpad in ia32.

There's a clang bug which causes crashes of syscalls which use many
registers for argument passing in ia32 architecture.
https://llvm.org/bugs/show_bug.cgi?id=16830

BUG=556393
==========

to

==========
[Linux] Turn off -mstackrealign for breakpad in ia32.

There's a clang bug which causes crashes of syscalls which use many
registers for argument passing in ia32 architecture.
https://llvm.org/bugs/show_bug.cgi?id=16830

BUG=556393

Committed: https://crrev.com/176ff2c7a3285a851a534603e2e893664ec5ace7
Cr-Commit-Position: refs/heads/master@{#365770}
==========

[commit-bot: I haz the power, 2015-12-17 07:05:13]

Patchset 3 (id:??) landed as
https://crrev.com/176ff2c7a3285a851a534603e2e893664ec5ace7
Cr-Commit-Position: refs/heads/master@{#365770}

[Roland McGrath, 2016-01-12 23:44:24]

mcgrathr@chromium.org changed reviewers:
+ dpranke@chromium.org, mcgrathr@chromium.org

[Roland McGrath, 2016-01-12 23:44:24]

The GN parts of this change are entirely bogus and were clearly never tested. 
There are no bots these days that test a GN build with target_cpu="x86" and
is_clang=true.  Because of the BUILD.gn changes here, the 'gn gen' step now
fails for target_cpu="x86" is_clang=true.

The intent appears to have been a direct transliteration of the GYP changes to
GN.  But variables like cflags are handled fundamentally differently in GN than
in GYP, and this approach is not viable at all in GN.  To conditionally remove
switches from cflags, what you really need to do is refactor the configs so that
the cflags elements in question come from a distinct config, and then use
'configs -= [ "..." ]' to conditionally remove that config.

[Lei Zhang, 2016-01-12 23:53:24]

On 2016/01/12 23:44:24, Roland McGrath wrote:
> The GN parts of this change are entirely bogus and were clearly never tested. 
> There are no bots these days that test a GN build with target_cpu="x86" and
> is_clang=true.  Because of the BUILD.gn changes here, the 'gn gen' step now
> fails for target_cpu="x86" is_clang=true.
> 
> The intent appears to have been a direct transliteration of the GYP changes to
> GN.  But variables like cflags are handled fundamentally differently in GN
than
> in GYP, and this approach is not viable at all in GN.  To conditionally remove
> switches from cflags, what you really need to do is refactor the configs so
that
> the cflags elements in question come from a distinct config, and then use
> 'configs -= [ "..." ]' to conditionally remove that config.

Doh. Do you have a fix in the works already, or should I fix this?

[Lei Zhang, 2016-01-13 00:59:49]

On 2016/01/12 23:53:24, Lei Zhang wrote:
> Doh. Do you have a fix in the works already, or should I fix this?

I'll just put together a CL to fix this.

[Roland McGrath, 2016-01-15 22:52:00]

On 2016/01/13 00:59:49, Lei Zhang wrote:
> On 2016/01/12 23:53:24, Lei Zhang wrote:
> > Doh. Do you have a fix in the works already, or should I fix this?
> 
> I'll just put together a CL to fix this.

I did https://codereview.chromium.org/1598523002 for this.