manoj, could you please help with this?

If I switch Makefile to use O1 instead Os, the unit tests pass.

rc/platform/vboot_reference/Makefile
DEBUG_FLAGS := $(if ${DEBUG},-g -O0,-Os) 

Os -> Fail
O2 -> Fail
O0 and O1 -> Pass

I switched the compiler to clang while still using binutils 2.25 and the unit test failed with identical error msgs.
So I do not think this is related to binutils except that newer binutils exposed the problem with gcc. 

tests/vboot_api_kernel5_tests.c:186: Image good ... FAILED
  Expected: 0x0 (0), got: 0x10010 (65552)
tests/vboot_api_kernel5_tests.c:187:   part num ... PASSED
tests/vboot_api_kernel5_tests.c:188:   bootloader addr ... FAILED
  Expected: 0xbeadd008 (-1095905272), got: 0x0 (0)
tests/vboot_api_kernel5_tests.c:189:   bootloader size ... FAILED
  Expected: 0x1234 (4660), got: 0x0 (0)
tests/vboot_api_kernel5_tests.c:191: kparams.kernel_buffer == (void *)(kernel_body_start),   kernel buffer ... FAILED
  Expected: 0x5576d54d06f0, got: 0x0
tests/vboot_api_kernel5_tests.c:193:   kernel buffer size ... FAILED
  Expected: 0x12880 (75904), got: 0x0 (0)
tests/vboot_api_kernel5_tests.c:199: Empty image ... PASSED
tests/vboot_api_kernel5_tests.c:204: Illegal image size ... PASSED
tests/vboot_api_kernel5_tests.c:211: Key verify failed ... PASSED
tests/vboot_api_kernel5_tests.c:212:   hash check ... FAILED
  Expected: 0x0 (0), got: 0xffffffff (-1)
tests/vboot_api_kernel5_tests.c:221: Key verify failed ... PASSED
tests/vboot_api_kernel5_tests.c:222:   hash check ... PASSED
tests/vboot_api_kernel5_tests.c:232: Key verify failed ... PASSED
tests/vboot_api_kernel5_tests.c:233:   hash check -- VBNV flag ... PASSED
tests/vboot_api_kernel5_tests.c:244: Developer flag mismatch - dev switch on ... PASSED
tests/vboot_api_kernel5_tests.c:256: Developer flag mismatch - dev switch on(gbb override) ... FAILED
  Expected: 0x0 (0), got: 0x10010 (65552)
tests/vboot_api_kernel5_tests.c:268: Recovery flag mismatch - dev switch on(gbb override) ... FAILED
  Expected: 0x0 (0), got: 0x10010 (65552)
tests/vboot_api_kernel5_tests.c:278: Developer flag mismatch - dev switch off ... PASSED
tests/vboot_api_kernel5_tests.c:288: Recovery flag mismatch ... PASSED
tests/vboot_api_kernel5_tests.c:295: Preamble verification ... PASSED
tests/vboot_api_kernel5_tests.c:302: Data verification ... PASSED

interesting experiment clang + binutils 2.25.

Maybe you could consult with the owner of the test? Maybe there were similar failures in the past.

Appears to be a case of undefined behavior (ODR rule violation):

Function vb2_unpack_key_buffer (and a few more functions) are defined in two locations:
tests/vboot_api_kernel5_tests.c and ./firmware/lib20/packed_key.c

The UnitTest expect the definition in tests/vboot_api_kernel5_tests.c to be used. But compiler/linker picks the ./firmware/lib20/packed_key.c with -O2 and 
tests/vboot_api_kernel5_tests.c with -O1.

If I add __attribute__((weak)) to the definition in ./firmware/lib20/packed_key.c, the UnitTests pass.

Assign to rspangler@ to take a look.

This is a blocker for binutils 2.27 upgrade. Bumping priority to P1.

Fix: https://chromium-review.googlesource.com/527601

this fixes one instance. 
are there any other routines that are mocked and need the same treatment?

Ping rspangler@ .

We pretty much mock *every* routine in the library.

But I'd really prefer to avoid making them all weak.

Previously, this worked:

# Allow multiple definitions, so tests can mock functions from other libraries
${BUILD}/tests/%: CFLAGS += -Xlinker --allow-multiple-definition

Why does this no longer work?

We are investigating the linker behavior change. 

It turns out that the root cause for this issue is an *assembler* behavior change.

The culprit is https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8dcea93252a9ea7dff57e85220a719e2a5e8ab41 which enables the assembler to "optimize out non-PLT relocations against defined non-weak global branch targets with default visibility".

It seems to do this only for JMP instructions, not CALL instructions. However, gcc optimizes CALL instructions to JMP instructions under certain conditions ("-Os"), which then triggers this optimization in the assembler.

Once the assembler has optimized away the relocation for "vb2_unpack_key_buffer", the linker doesn't see it, and therefore can't resolve it to the mock function in the test source.

This also explains why adding attribute(weak) makes the test pass: the attribute disables the optimization for this symbol in the assembler.

I've also verified that adding "-Xassembler -mshared" to CFLAGS in the vboot_reference Makefile makes the test pass with binutils-2.27.

Thanks Rahul. 
This is similar to the behavior if compiler decides to inline the functions that we discussed.
e.g. A.c has foo() and bar() with foo() calling bar().
if compiler decides to inline bar(), then foo() will have a call to bar() at runtime. And if bar() is mocked, it is not going to have any effect.

So the weak linkage attribute is necessary here.

Another option is to put the mocked functions in separate C files so that compiler/assembler can't optimize any calls to them.

Correction to #13:
if compiler decides to inline bar(), then foo() will *NOT* have a call to bar() anymore. So if bar() is mocked, it is not going to have any effect.

Some more context, this time with the effects of various flags/attributes:

The function in question is "vb2_unpack_key", defined in firmware/lib20/packed_key.c:

int vb2_unpack_key(struct vb2_public_key *key,
                   const struct vb2_packed_key *packed_key)
{
        if (!packed_key)
                return VB2_ERROR_UNPACK_KEY_BUFFER;

        return vb2_unpack_key_buffer(key,
                                     (const uint8_t *)packed_key,
                                     packed_key->key_offset +
                                     packed_key->key_size);
}

It calls vb2_unpack_key_buffer(), which is defined in the same file, and a mock version is defined in tests/vboot_api_kernel5_tests.c. We expect the regular binaries to use the function in packed_key.c, but the test binaries to use the function in vboot_api_kernel5_tests.c.

When "-Wa,-mshared" flag is added to CFLAGS, "vb2_unpack_key" gets compiled as:

00000000000000c7 <vb2_unpack_key>:
  c7:   48 85 f6                test   rsi,rsi
  ca:   74 0a                   je     d6 <vb2_unpack_key+0xf>
  cc:   8b 56 08                mov    edx,DWORD PTR [rsi+0x8]
  cf:   03 16                   add    edx,DWORD PTR [rsi]
  d1:   e9 00 00 00 00          jmp    d6 <vb2_unpack_key+0xf>
                        d2: R_X86_64_PC32       vb2_unpack_key_buffer-0x4
  d6:   b8 2a 00 05 10          mov    eax,0x1005002a
  db:   c3                      ret

i.e. it has a relocation for the "vb2_unpack_key_buffer" that gets resolved by the linker to the mocked version in the test binary. This is the same behavior as binutils-2.25, and the test passes.

Without "-Wa,-mshared" flag, "vb2_unpack_key" gets compiled as:

00000000000000c7 <vb2_unpack_key>:
  c7:   48 85 f6                test   rsi,rsi
  ca:   74 0a                   je     d6 <vb2_unpack_key+0xf>
  cc:   8b 56 08                mov    edx,DWORD PTR [rsi+0x8]
  cf:   03 16                   add    edx,DWORD PTR [rsi]
  d1:   e9 43 ff ff ff          jmp    19 <vb2_unpack_key_buffer>
  d6:   b8 2a 00 05 10          mov    eax,0x1005002a
  db:   c3                      ret

i.e. the assembler in binutils-2.27 has optimized away the relocation. The linker doesn't get to resolve the symbol to the mock version in the test binary. The test fails.

If, instead of adding "-Wa,-mshared" to CFLAGS, we add "__attribute__((weak))" to the definition of "vb2_unpack_key_buffer" in packed_key.c, "vb2_unpack_key" gets compiled as:

00000000000000c7 <vb2_unpack_key>:
  c7:   48 85 f6                test   rsi,rsi
  ca:   74 0a                   je     d6 <vb2_unpack_key+0xf>
  cc:   8b 56 08                mov    edx,DWORD PTR [rsi+0x8]
  cf:   03 16                   add    edx,DWORD PTR [rsi]
  d1:   e9 00 00 00 00          jmp    d6 <vb2_unpack_key+0xf>
                        d2: R_X86_64_PLT32      vb2_unpack_key_buffer-0x4
  d6:   b8 2a 00 05 10          mov    eax,0x1005002a
  db:   c3                      ret

i.e. in this case, the call to vb2_unpack_key_buffer goes through the PLT, and the assembler leaves this relocation alone. The test passes.

The assembler behavior seems correct here, and the source and/or Makefile should be modified to get the correct mocking behavior in tests. As Manoj suggested, you can:

1) Add "__attribute__((weak))" to all functions that are going to be mocked in tests, or
2) Move all functions that are going to be mocked into separate C source files, so the assembler won't be able to do this optimization, or
3) Add "-Wa,-mshared" to CFLAGS in the Makefile to disable this optimization in the assembler as a workaround. This may look confusing, as the object files are not really meant to be included in a shared library.

Randall, ping? 

we are very ready to roll binutils and this is quickly becoming a blocker. 
Could you please take care of this ASAP?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/3522e574a21a65ae4ef76ac396101c68e1c985a1

commit 3522e574a21a65ae4ef76ac396101c68e1c985a1
Author: Randall Spangler <rspangler@chromium.org>
Date: Wed Jun 21 00:24:20 2017

2lib: Add test_mockable attribute

Some tests mock library functions.  This previously worked due to adding

  CFLAGS += -Xlinker --allow-multiple-definition

to the test binaries.  But the new version of binutils seems to need
the default implementation to be weak if compiled with -O2 in some
cases.  Add test_mockable for use with functions where this is now
needed.

BUG= chromium:723906 
BRANCH=none
TEST=Add CFLAGS += -O2 to the makefile, then make -j runtests
     Tests break before this change with -O2, and work afterwards

Change-Id: I95996a3e1086251442055765295a75de4c20ee3c
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/527601
Commit-Ready: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Rahul Chaudhry <rahulchaudhry@chromium.org>
Reviewed-by: Rahul Chaudhry <rahulchaudhry@chromium.org>

[modify] https://crrev.com/3522e574a21a65ae4ef76ac396101c68e1c985a1/firmware/lib20/packed_key.c
[modify] https://crrev.com/3522e574a21a65ae4ef76ac396101c68e1c985a1/firmware/2lib/include/2common.h
[modify] https://crrev.com/3522e574a21a65ae4ef76ac396101c68e1c985a1/firmware/include/vboot_api.h

I'm already tied up with other P0/P1 bugs.  I won't have time to really look at this for weeks.  +reinauer to identify another firmware resource if needed.

Confused. I thought you had fixed the issue.

I made a workaround for the one function referenced in the original bug.  Someone pushed my change today, despite the concerns this is the wrong fix.

The underlying issue that the new assembler optimizes function calls differently is not fixed.

I put the CL in commit-queue yesterday after fixing the compile error related to duplicate definition of the test_mockable macro.

The concern was not that it's the wrong fix: adding __attribute__(weak) is one of the suggestions in comment #15 and before.

The concern was that other functions that are mocked in unit tests probably need that attribute as well. Otherwise, future compiler/toolchain changes (or simply refactoring code in the package) might expose this same issue with other unit tests.

How you file, track, and prioritize that cleanup is up to you and your team. You're obviously busy with higher priority items right now.

On the other hand, your CL does fix the one unittest failure that was exposed by binutils-2.27. We can move forward with the binutils upgrade now (the upgrade window is closing, as it needs to be done early in the chromiumos release cycle).

Got it, thanks.

Closing. Please reopen it if its not fixed. Thanks!