1415782 - [clang 5 ASAN] LulIntegration.unwind_consistency | Value of: nTestsPassed == nTests

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Mike Hommey [:glandium]]

When doing a try push with the patch from bug 1409265 applied, gtest fails on linux64 asan builds:

https://treeherder.mozilla.org/logviewer.html#?job_id=143211291&repo=try&lineNumber=16817

[task 2017-11-09T03:17:17.362Z] 03:17:17  WARNING -  TEST-UNEXPECTED-FAIL | LulIntegration.unwind_consistency | Value of: nTestsPassed == nTests
[task 2017-11-09T03:17:17.363Z] 03:17:17     INFO -    Actual: false
[task 2017-11-09T03:17:17.363Z] 03:17:17     INFO -  Expected: true
[task 2017-11-09T03:17:17.363Z] 03:17:17     INFO -  Not all tests passed @ /builds/worker/workspace/build/src/tools/profiler/tests/gtest/LulTest.cpp:48
[task 2017-11-09T03:17:17.440Z] 03:17:17  WARNING -  TEST-UNEXPECTED-FAIL | LulIntegration.unwind_consistency | test completed (time: 5086ms)

Comment 1

[Christian Holler (:decoder)]

This test seems to be the only blocker for upgrading ASan to either 5 or even 6 (current trunk).

In case this problem is harder to fix, would it be ok to disable this feature on ASan builds? Would it suffice to just disable the test? I don't want to block an important project on fixing this one test if it can't be done quickly and isn't strictly required. Thanks for looking into this!

Comment 2

[Julian Seward [:jseward]]

There are actually two different problems:

* LUL doesn't support Dwarf version 4 CFI (unwind info)

* The test case is broken by Clang 5's optimisations (and may have been
  broken with other compilers -- I don't know.)

I'll attach patches for both.

Comment 3

[Julian Seward [:jseward]]

Attachment: Adds minimal Dwarf 4 CFI support to LUL

Comment 4

[Julian Seward [:jseward]]

Attachment: Fix unwind gtest cases, so they work with Clang 5

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8927665 [details] [diff] [review]
Fix unwind gtest cases, so they work with Clang 5

Review of attachment 8927665 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/lul/LulMain.cpp
@@ +1631,1 @@
>      volatile char space[FRAMESIZE]; \

This seems like an awful lot of fragile stuff to avoid the compiler trying to be too smart. Why not "simply" use alloca(FRAMESIZE)? I don't think that's the kind of thing the compiler should be allowed to optimize out.

Comment 6

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Why not "simply" use alloca(FRAMESIZE)? I don't think
> that's the kind of thing the compiler should be allowed to optimize out.

Do you have some reason to believe the compiler isn't allowed to optimise
out unused alloca-space to the same extent that it can optimise out unused
local arrays?  I tried clang 5 at -O with the code below and in both cases
the array is removed.

  #include <alloca.h>
  #include <string.h>

  int f1 (void) {
     volatile int arr[100];
     memset((void*)arr, 0, sizeof(arr));
     return arr[0];
  }

  int f2(void) {
     volatile int* arr = alloca(100 * sizeof(int));
     memset((void*)arr, 0, 100 * sizeof(int));
     return arr[0];
  }

produces

  f1:                                     # @f1
	  .cfi_startproc
  # BB#0:
	  movl	$0, -8(%rsp)
	  movl	-8(%rsp), %eax
	  retq

  f2:                                     # @f2
	  .cfi_startproc
  # BB#0:
	  movl	$0, -8(%rsp)
	  movl	-8(%rsp), %eax
	  retq

Removing the volatile qualifiers makes the resulting code even shorter, in
both cases simply "xorl %eax, %eax ; ret".

Comment 7

[Julian Seward [:jseward]]

(In reply to Julian Seward [:jseward] from comment #6)
> in both cases the array is removed.

Given that, it seems to me that regardless of whether alloca or "normal"
allocation is used, we will have to jump through the same hoops with
inline assembly in order to convince clang not to delete the space.  On
that basis, are you happy to continue to review with the patch as it is?

Comment 8

[Mike Hommey [:glandium]]

How about calling a function that is not in the compilation unit and that will do nothing with the buffer? A no-op like write(1, buf, 0); or something in the same vein.

Comment 9

[Mike Hommey [:glandium]]

It seems to work, without volatile, and without a memset.

Comment 10

[Julian Seward [:jseward]]

(In reply to Mike Hommey [:glandium] from comment #8)
> How about calling a function that is not in the compilation unit and that
> will do nothing with the buffer?

Well, that adds complexity in the need to have a new function and a new
compilation unit.  I don't see that the current scheme is fragile.  The
compiler can't move the inline assembly around because it is marked 
__volatile__, and it's not machine dependent, because there are no
instructions in the assembly.  So the only dependency is on a compiler
that supports inline assembly in this style, which is at least gcc, clang
and icc.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Julian Seward [:jseward] from comment #10)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > How about calling a function that is not in the compilation unit and that
> > will do nothing with the buffer?
> 
> Well, that adds complexity in the need to have a new function and a new
> compilation unit.

You don't have to add a new function and a new compilation unit. As I said, you can use an existing one that does nothing with the given arguments, and is in a different compilation unit. write is an example of a such a function. There might be functions with even less overhead. By using such a function, you don't even need add a loop that tries to initialize the buffer in a way that the compiler is not going to optimize out.

Comment 12

[Julian Seward [:jseward]]

Attachment: Fix unwind gtest cases, so they work with Clang 5

Now redone using write() and mozilla::Unused.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8929576 [details] [diff] [review]
Fix unwind gtest cases, so they work with Clang 5

Review of attachment 8929576 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/lul/LulMain.cpp
@@ +1729,2 @@
>        __asm__ __volatile__("":::"cc","memory"); \
> +      Unused << write(1, space, 0); \

This one seems overzealous.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb0aa423e51
[clang 5 ASAN] LulIntegration.unwind_consistency | Value of: nTestsPassed == nTests (part 1 of 2).  r=njn.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6443c19376bc
[clang 5 ASAN] LulIntegration.unwind_consistency | Value of: nTestsPassed == nTests (part 2 of 2).  r=glandium.

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3eb0aa423e51
https://hg.mozilla.org/mozilla-central/rev/6443c19376bc