Add mock test for rcutils/strerror #265

Blast545 · 2020-07-03T20:00:20Z

This PR includes a test for the rcutils_strerror function that required making a mock of the strerror_r / strerror_s.

Signed-off-by: Jorge Perez [email protected]

hidmic · 2020-07-08T22:37:14Z

test/test_strerror.cpp

+#include <string>
+#else
+#include <string>
+#endif


@brawner @wjwwood @clalancette this is a non-portable workaround for an issue @Blast545 and I stumbled upon. Turns out librcutls.so and this test's executable where linking against different strerror_r versions. Different libc versions, and different symbol names (strerror_r is actually a macro for the libc version librcutils.so links to on Ubuntu 20.04).

This seems like a significant pain point whenever mocking system libraries if it turns out such practices are widespread (and I'd think so). We need a way to ensure both test and library use and link against the same third party OR use a wrapper with a stable ABI instead of system libraries directly.

@brawner @wjwwood @clalancette this is a non-portable workaround for an issue @Blast545 and I stumbled upon. Turns out librcutls.so and this test's executable where linking against different strerror_r versions.

Huh, weird. Can you explain a little more of the details? I'm just trying to get a handle on how common this will be.

holiday mode OFF

Can you explain a little more of the details? I'm just trying to get a handle on how common this will be.

Maybe ELF dumps help.

Output fragment of readelf --relocs path/to/librcutils.so:

Relocation section '.rela.plt' at offset 0x3480 contains 107 entries: Offset Info Type Sym. Value Sym. Name + Addend ... 0000000180c8 000e00000007 R_X86_64_JUMP_SLO 0000000000000000 __xpg_strerror_r@GLIBC_2.3.4 + 0 000000018358 007000000007 R_X86_64_JUMP_SLO 000000000000e250 rcutils_strerror + 0 ...

Output fragment of readelf --relocs path/to/test_strerror:

Relocation section '.rela.plt' at offset 0x6780 contains 206 entries: Offset Info Type Sym. Value Sym. Name + Addend ... 0000000799e8 001700000007 R_X86_64_JUMP_SLO 0000000000000000 strerror@GLIBC_2.2.5 + 0 000000079bd8 005700000007 R_X86_64_JUMP_SLO 0000000000000000 rcutils_strerror + 0 ...

Relevant glibc string.h header section.

I don't know exactly why this happens, though the C++ runtime may have something do with it (librcutils.so is C only).

holiday mode on

@Blast545 we need a solution for this. Playing with system library defines is brittle and non-portable.

Digging a bit, it turns out that it's libstdc++ that requires _GNU_SOURCE to be set (see here). It's admittedly a bad workaround. We could set _GNU_SOURCE when building rcutils on Linux. @clalancette @dirk-thomas @wjwwood for feedback.

After talking to @hidmic about this offline, I offered this opinion:

I think so long as a) we do not require these GNU specific functions (graceful degradation to POSIX or Windows is possible) and b) they (the GNU functions) are available, then it is fine to use them or force it to use them.

I don't have a sense for any intrinsic drawback or advantage to using the GNU ones over the POSIX ones.
Like performance or security benefits, but they could convince me otherwise, if they exist.

Ultimately, the problem's in g++, but the bug's been standing for 19 years and is tagged as wontfix. It also creates some fun booby traps in C++ code I wasn't aware off e.g. you can't use the stl and name a free function minor, it'll clash with some GNU macro.

For the most part, I agree with @wjwwood 's assessment. My only concern is about booby-traps like @hidmic laid out in the last comment.

There are specific areas I'm worried about:

The cross-platform area. If you use a non-portable GNU API by accident, will it obviously show up (fail to compile or link) on the other platforms?

The name clash areas. As @hidmic found out, it looks like the GNU API is "wider" than the POSIX one. Is it obvious when you run into one of these clashes? Have we tried running a full CI run to see what happens?

Assuming we are happy with the answers to those questions, then yeah, I'm fine with going ahead with the GNU API on Linux.

The cross-platform area. If you use a non-portable GNU API by accident, will it obviously show up (fail to compile or link) on the other platforms?

In general, yes. But there are other cases, more subtle, that we have to bear in mind.

The name clash areas. As @hidmic found out, it looks like the GNU API is "wider" than the POSIX one. Is it obvious when you run into one of these clashes? Have we tried running a full CI run to see what happens?

So far, rcutils builds and tests succeed locally. That does not guarantee we won't ever come across a name clash (with a macro, other redefinitions will be caught by the compiler).

Nevermind the choice we make here, I'd want to stress the fact that, because of what g++ does when it finds glibc in the system, in general we cannot ignore GNU API peculiarities. A C++ program using getopt is potentially broken already if these are not taken into account.

Nevermind the choice we make here, I'd want to stress the fact that, because of what g++ does when it finds glibc in the system, in general we cannot ignore GNU API peculiarities. A C++ program using getopt is potentially broken already if these are not taken into account.

Actually, that brings up another point to consider here. I don't remember the exact way to do it in cmake/ament, but I think we need to "export" -D_GNU_SOURCE to downstream packages so that they also compile properly. Does that sound right to you?

That's probably the safest bet, but I fear we're spreading the disease. Should a downstream package that uses rcl be exposed to GNU extensions? Looking at glibc documentation, _GNU_SOURCE introduces LFS extensions that are no-op in 64 bits but have side effects in 32 bits. If I'm in 32 bits, I understand POSIX limitations, and having a dependency hijacking my carefully crafted code is not good.

What if we just make sure we don't do anything stupid like using off_t in a function signature?

brawner · 2020-07-09T18:05:30Z

test/test_strerror.cpp

+  /* Tell the mock to return NULL and set errno to ENOMEM
+     whatever the given parameter is. */
+  mmk_when(
+    strerror_r(mmk_any(int), mmk_any(char *), mmk_any(size_t) ),


This sets the global behavior of the mocked function? Are there reasonable scenarios where you might want the default behavior of the mocked function generally, but have the mocked version for one specific call? How might you accomplish that?

Yeah, that's something I thought about, but unfortunately Mimick mocks do not explicitly support that feature. You could recreate behavior by building a table of mmk_when expressions, but it's impractical and brittle. I only know of gMock for a framework that can do that though.

Uh, turns out it has the feature and @Blast545 found it 🎉

brawner · 2020-07-09T18:24:17Z

test/test_strerror.cpp

+  errno = 2;
+  char error_string[1024];
+  rcutils_strerror(error_string, sizeof(error_string));
+  ASSERT_STREQ(error_string, "Failed to get error");


I'm sure this is a case-by-case matter, but what risks are there for maintainers adding calls to the mocked function that sits between the unit test and the targeted invocation? It seems like unit tests that call high-level functions, but mock low-level functions would be pretty brittle, and you would need to mock functions whose number of uses in the code under test is unlikely to change. rcutils_strerror is a good example where the number of uses of strerror_r are unlikely to change. Whereas mocking something like rcutils_strndup during complex code like rclcpp::init would result in behaviors that are difficult to assert.

I don't see a fundamental risk, besides the maintenance burden that comes from white-box testing and the coupling between test and implementation it entails. It's important to bear in mind that mocking is not a tool for fault injection in a running system, but a way to unit test the interactions between the entity under test and its direct dependencies. In an rclcpp::init test, one would mock rcutils_strndup along with all other direct dependencies to ensure correct behavior.

test/test_strerror.cpp

hidmic · 2020-07-14T21:36:42Z

test/test_strerror.cpp

+  // mmk_mock("strerror_r@rcutils", strerror_r_mock);
+  // mmk_mock("__xpg_strerror_r@self", strerror_r_mock);
+  // mmk_mock("strerror_r@self", strerror_r_mock);
+  mmk_mock("__xpg_strerror_r@lib:rcutils", strerror_r_mock);


@Blast545 hmm the symbol is non-portable. Maybe something like:

Suggested change

mmk_mock("__xpg_strerror_r@lib:rcutils", strerror_r_mock);

mmk_mock(RCUTILS_STRINGIFY(strerror_r) "@lib:rcutils", strerror_r_mock);

can do.

You mean that we could define another macro similar to RCUTILS_STRINGIFY? I don't understand this suggestion

I mean to let the compiler find out what strerror_r actual symbol is. If strerror_r is a macro that expands to something else, that'll get resolved before RCUTILS_STRINGIFY turns it into a string literal. Then we rely on C to concatenate string literals (in C, "foo" " bar" is the same as writing "foobar") and voilà! We get to write it in a portable way.

Mmm, any ideas on how to do that? that would be great, not sure if the compiler is able to find that actual symbol name.

Without anything else, it resolves to "strerror_r@lib:rcutils". We could write a macro that returns the hardcoded generated name in each platform if we find how that name is generated.

Nevermind what it expands to, I think it's more portable to let the compiler figure out what the symbol is instead of hardcoding it.

hidmic · 2020-07-14T21:47:39Z

test/test_strerror.cpp

+#include <string>
+#else
+#include <string>
+#endif


@Blast545 we need a solution for this. Playing with system library defines is brittle and non-portable.

Digging a bit, it turns out that it's libstdc++ that requires _GNU_SOURCE to be set (see here). It's admittedly a bad workaround. We could set _GNU_SOURCE when building rcutils on Linux. @clalancette @dirk-thomas @wjwwood for feedback.

Signed-off-by: Jorge Perez <[email protected]>

Blast545 · 2020-07-21T19:57:32Z

@hidmic I rewrote some of the tests to use the "then_return" functionality, I was worried about the cases that don't return any useful, but modify one of the passed arguments. Seems to be working really well!

test/test_strerror.cpp

hidmic · 2020-07-21T20:19:27Z

test/test_strerror.cpp

+  /* Tell the mock to return NULL and set errno to ENOMEM
+     whatever the given parameter is. */
+  mmk_when(
+    strerror_r(mmk_any(int), mmk_any(char *), mmk_any(size_t) ),


Uh, turns out it has the feature and @Blast545 found it 🎉

test/test_strerror.cpp

hidmic · 2020-07-21T20:47:11Z

BTW this PR needs a new name.

Signed-off-by: Jorge Perez <[email protected]>

Blast545 · 2020-07-22T14:47:27Z

@hidmic I addressed your suggestions, let me know what you think

hidmic

LGTM pending green CI

test/test_strerror.cpp

Signed-off-by: Jorge Perez <[email protected]>

Blast545 · 2020-07-23T21:06:27Z

CI:

Linux
Linux-aarch64
macOS (Fixed below)
Windows

Signed-off-by: Jorge Perez <[email protected]>

Blast545 · 2020-07-27T16:41:26Z

Windows (Problem with Windows case, I'm checking this out)

Blast545 · 2020-07-27T16:50:27Z

CI:

Linux
Linux-aarch64
macOS
Windows

Signed-off-by: Jorge Perez <[email protected]>

hidmic

LGTM !

Blast545 · 2020-07-28T21:12:35Z

Re triggering CI after change in mimick_vendor

Linux
Linux-aarch64
macOS
Windows

Blast545 · 2020-07-29T20:28:40Z

@ros-pull-request-builder retest this please

Blast545 · 2020-07-29T21:03:03Z

Tier 1 platforms green, PR job green + 1 reviewer, merging 🎉

* Add test using mimick vendored library * Add path to mimick_vendor * Link test to mimick library * Add multi-platform support * Reformat to separate different platform tests * Reformat to use strncpy to copy buffer * Change reset macro to take strerror_mock * Use mallock return type to reset function * Remove test_depend to mimick * Replace mock type * Change function strncpy to strncpy_s Signed-off-by: Jorge Perez <[email protected]>

hidmic mentioned this pull request Jul 8, 2020

[meta] Mocking infrastructure for C APIs ros2/ros2#960

Closed

3 tasks

hidmic reviewed Jul 8, 2020

View reviewed changes

brawner reviewed Jul 9, 2020

View reviewed changes

Blast545 marked this pull request as ready for review July 14, 2020 20:26

hidmic reviewed Jul 14, 2020

View reviewed changes

hidmic mentioned this pull request Jul 16, 2020

Force _GNU_SOURCE if glibc is used. #267

Merged

Blast545 added 7 commits July 20, 2020 19:14

Add test using mimick vendored library

ae91f73

Signed-off-by: Jorge Perez <[email protected]>

Add path to mimick_vendor

18b53d1

Signed-off-by: Jorge Perez <[email protected]>

Link test to mimick library

56782e7

Signed-off-by: Jorge Perez <[email protected]>

Fix library usage issues

2927385

Signed-off-by: Jorge Perez <[email protected]>

Fix mimick usage

7a7ec42

Signed-off-by: Jorge Perez <[email protected]>

Remove commented code

d3407d6

Signed-off-by: Jorge Perez <[email protected]>

Add multi-platform support

8338f9e

Signed-off-by: Jorge Perez <[email protected]>

Blast545 force-pushed the blast545/test_mimick branch from 924e386 to 8338f9e Compare July 21, 2020 17:32

Blast545 requested a review from hidmic July 21, 2020 19:56

hidmic reviewed Jul 21, 2020

View reviewed changes

Blast545 added 3 commits July 22, 2020 11:30

Reformat to separate different platform tests

2135205

Signed-off-by: Jorge Perez <[email protected]>

Remove not needed constant

4e8b05d

Signed-off-by: Jorge Perez <[email protected]>

Reformat to use strncpy to copy buffer

dcf80fa

Signed-off-by: Jorge Perez <[email protected]>

Blast545 changed the title ~~Mimick vendored library testing (WIP)~~ Add mock test for rcutils/strerror Jul 22, 2020

Blast545 requested a review from hidmic July 22, 2020 14:47

hidmic approved these changes Jul 22, 2020

View reviewed changes

test/test_strerror.cpp Outdated Show resolved Hide resolved

Replace comments /* with //

85d4d1a

Signed-off-by: Jorge Perez <[email protected]>

Remove not needed constant for MacOs

fe73174

Signed-off-by: Jorge Perez <[email protected]>

Blast545 added 9 commits July 27, 2020 13:59

Remove extra parameter windows call

5b8ce8f

Signed-off-by: Jorge Perez <[email protected]>

Cast function type

1b3ad34

Signed-off-by: Jorge Perez <[email protected]>

Add missing parentheses

fd8d93b

Signed-off-by: Jorge Perez <[email protected]>

Change reset macro to take strerror_mock

f00a0dc

Signed-off-by: Jorge Perez <[email protected]>

Use mallock return type to reset function

0ffe78b

Signed-off-by: Jorge Perez <[email protected]>

Remove test_depend to mimick

200b5e0

Signed-off-by: Jorge Perez <[email protected]>

Replace mock type

a870d60

Signed-off-by: Jorge Perez <[email protected]>

Change function strncpy to strncpy_s

ee41b34

Signed-off-by: Jorge Perez <[email protected]>

Add missing param

a6cfb76

Signed-off-by: Jorge Perez <[email protected]>

Blast545 requested a review from hidmic July 27, 2020 21:24

hidmic approved these changes Jul 27, 2020

View reviewed changes

hidmic mentioned this pull request Jul 28, 2020

Complete rcutils API coverage #272

Merged

Blast545 merged commit eabfb01 into master Jul 29, 2020

delete-merged-branch bot deleted the blast545/test_mimick branch July 29, 2020 21:04

	mmk_mock("__xpg_strerror_r@lib:rcutils", strerror_r_mock);
	mmk_mock(RCUTILS_STRINGIFY(strerror_r) "@lib:rcutils", strerror_r_mock);

Add mock test for rcutils/strerror #265

Add mock test for rcutils/strerror #265

Uh oh!

Conversation

Blast545 commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Blast545 commented Jul 21, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hidmic commented Jul 21, 2020

Uh oh!

Blast545 commented Jul 22, 2020

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Blast545 commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Blast545 commented Jul 3, 2020 •

edited

Loading

hidmic Jul 14, 2020 •

edited

Loading

hidmic Jul 16, 2020 •

edited

Loading

clalancette Jul 17, 2020 •

edited

Loading

hidmic Jul 17, 2020 •

edited

Loading

hidmic Jul 13, 2020 •

edited

Loading

hidmic Jul 17, 2020 •

edited

Loading

hidmic Jul 14, 2020 •

edited

Loading

Blast545 commented Jul 23, 2020 •

edited

Loading

Blast545 commented Jul 27, 2020 •

edited

Loading

Blast545 commented Jul 27, 2020 •

edited

Loading