ThrowTheSwitch / CMock

CMock - Mock/stub generator for C
http://throwtheswitch.org
MIT License
653 stars 269 forks source link

Inline functions being mocked despite treat_inlines being set to :exclude #427

Closed JonTBeckett closed 1 year ago

JonTBeckett commented 1 year ago

Hi, I'm trying to do some unit testing that involves mocking the PSA crypto API provided in the Trusted Firmware for M project. The problem is that there are inline functions in the crypto.h header file that are being mocked despite having :treat_inlines explicitly set to :exclude in my project.yml (for Ceedling). See the Cmock settings below:

:cmock:
  :treat_inlines:
    :exclude
  :mock_prefix: mock_
  :when_no_prototypes: :warn
  :enforce_strict_ordering: TRUE
  :plugins:
    - :ignore
    - :callback
  :treat_as:
    uint8:    HEX8
    uint16:   HEX16
    uint32:   UINT32
    int8:     INT8
    bool:     UINT8

To be more exact, the crypto.h file includes crypto_struct.h which defines the inline functions. When I mock crypto.h (by placing #include "mock_crypto.h" in my test file) even the inline functions are being mocked.

I set up a super simple recreation of this issue in this repository: https://github.com/JonTBeckett/mock-tfm-demo

I'm using the following command to setup the docker container for running ceedling: docker run -it --rm -v <root of repo linked above>:/project throwtheswitch/madsciencelab

I am using the following command to run the test and generate the error: ceedling clobber test:all

Below is an example of one of the many identical errors revolving around inline functions.

build/test/mocks/mock_crypto.c:10330:32: error: redefinition of 'psa_key_derivation_operation_init'
10330 | psa_key_derivation_operation_t psa_key_derivation_operation_init(void)
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../trusted-firmware-m/interface/include/psa/crypto.h:4032,
                 from build/test/mocks/mock_crypto.h:6,
                 from build/test/mocks/mock_crypto.c:6:
../trusted-firmware-m/interface/include/psa/crypto_struct.h:101:43: note: previous definition of 'psa_key_derivation_operation_init' was here
  101 | static inline struct psa_key_derivation_s psa_key_derivation_operation_init( void )
      |                                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Am I missing something in my setting that is causing the inline functions to be mocked despite explicitly setting them to be excluded?

Thanks for any help!

mvandervoord commented 1 year ago

The :treat_inlines: should be :treat_inlines: :exclude (all on a single line). Your version becomes a string. The correct format becomes a symbol in Ruby.

If you still have problems after that, you might consider disabling preprocessing in Ceedling to see if your preprocessor is mucking around in your file ahead of time, but I suspect it's the formatting issue above.

JonTBeckett commented 1 year ago

Thanks for the quick response!

I had already tried different formats for the yaml setting but went back this morning and did the single line version one more time just to confirm. The issue is still there even with the single line format.

I'll tinker with the Ceedling precompile and try to run CMock outside of Ceedling today. Thanks again for the quick response and a bit of a direction with it!

JonTBeckett commented 1 year ago

I updated my Ceedling project.yml to set :use_test_preprocessor: FALSE and the issue still persists.

I'm still working on running CMock outside of Ceedling. I'm unable to run with the simple ruby cmock.rb command in the Ceedling Docker environment and haven't had a chance to setup my environment with Ruby to install it manually.

JonTBeckett commented 1 year ago

Finally got a chance to figure out how to run CMock in the docker container and ran the following command: ruby /usr/local/bundle/gems/ceedling-0.31.1/vendor/cmock/lib/cmock.rb /project/trusted-firmware-m/interface/include/psa/crypto.h The generated output is now in the demo/mocks directory in the repo I linked in the issue description.

If my understanding is correct, the inline functions should be excluded by defult but the above command still generates mocks for the inlines.

mvandervoord commented 1 year ago

When issuing the command to run cmock manually, you'll need to give it your project.yml file, like so:

ruby /usr/local/bundle/gems/ceedling-0.31.1/vendor/cmock/lib/cmock.rb -o"project.yml" /project/trusted-firmware-m/interface/include/psa/crypto.h

I don't think that's your problem, though, because the default behavior of CMock is to exclude inlines.

The problem you're running into is that your architecture does a lot of sneaking includes. You're including mock_crypto in your test file, which is creating a mock of the non-inline'd function psa_key_derivation_operation_init. This file also includes crypto_struct.h waaaaaaaay at the bottom. This second header file is being included by the header file you've mocked... but it's not getting mocked itself... so now you have a real version of the file competing with an inline version of the file.

The interesting thing about this is that I suspect you don't have most of your warnings enabled with your toolchain. Your compiler when you're building a release should ALSO complain about this. Declaring a non-inline function and then implementing an inline version of that same file should be a warning at a minimum... and honestly most compilers will flag it as an error (like the one is doing for your tests).

This isn't really a CMock issue. It's uncovered an error in your code.

However, I suspect that you're stuck with this library you're pulling in for the test, and often you can't fix bad libraries. The workaround to getting your tests working is to make a light version of crypto.h. You clone it, strip out all the ugly includes, throw in a few basic types (they probably don't even need to be the real types... often single ints will suffice) and the rest of your api is now mockable.

Best of luck!

JonTBeckett commented 1 year ago

Yeah, TFM (the library) is a bit interesting. It's an implementation of the Platform Security Architecture API specification which is why the crypto.h file is awkward that way. The crypto.h file has requirements from the PSA API Spec and then I'm guessing that crypto_struct.h is where/how TFM chose to implement that API. My quick and dirty solution was exactly what you suggested, create a trimmed down version of crypto.h. This works but I was hoping to utilize CMock a bit more to avoid that.

So the declarations in crypto.h are static. The way I read the summary markdown, I understand that static functions are lumped in with inline functions based on the default pattern. Is this accurate? If this is the case, I wouldn't expect CMock to mock those functions anyway.

As for warnings, we use MISRA C compatible warnings. Which it does slightly surprise me that online definitions are allowed after non-inline declarations but it is what it is.

JonTBeckett commented 1 year ago

I've tried multiple compilers and none care that this type of thing is happening in a header file:

// declaration
static void func(void);

// Definition
static inline void func(void)
{
    int x = 0;
}

The issue is definitely that CMock is mocking the static inline functions, causing multiple definitions of the same function. If CMock didn't mock the inline functions then it could safely include the psa/crypto_struct.h file in the mock_crypto.h file without any compilation issues.

The way I read the summary markdown, CMock shouldn't be mocking any function with a static, static inline, inline static, or inline modifier before it. Is that correct? I'm slowly reading through the CMock code when I can but I've never worked with Ruby and only have a couple of minutes here and there to do it so it is slow moving.

Letme commented 1 year ago

You can also define other attributes in the detection of static inline.

What you say, we split the bottom function implementation into a separate header file and guard its include with ifdef. But maybe you have some more complex inline implementation?

JonTBeckett commented 1 year ago

@Letme, I'm just trying to determine if there is an issue with CMock, my configuration, or if this just isn't supported by CMock. The way I read the summary markdown, both the static void func(void); declaration AND the static inline void func(void) definition should be ignored by CMock with just the default configuration. CMock is currently mocking the static function based on the declaration with the default configuration.

The question then is do I need to alter my configuration or is this an issue with CMock that I will have to just work around?

Letme commented 1 year ago

I thought you have a typo by having static without inline in header file and then followed by static inline. I do not think we have such case, since static, but not inline functions have no place to be in header (kinda you want local scope, but expose it outside?). I would be stripping that static by defining strippables. Can you paste generated mock_crypto.h? Because I think it does not realize it is an inline function now...

JonTBeckett commented 1 year ago

So the summary markdown states the following, which I take to mean that it does NOT mock functions declared static in header files.

:exclude will ignore inlined functions (default). CMock will look for the following default patterns (simplified from the actual regex):

"static inline" "inline static" "inline" "static" You can override these patterns, check out :inline_function_patterns.

JonTBeckett commented 1 year ago

Next time I am on my laptop I will try to generate the mock_crypto.h and paste it in.

I will look into strippables as a better work around as well.

Edit: mock_crypto.h is way too large to paste here. Feel free to read above, clone the linked repo, and generate it locally.

JonTBeckett commented 1 year ago

So I simplified the example. The following is the CMock configuration:

:cmock:
  :strippables: 
    - "static"
    - "^\\s*static.*"
  :treat_inlines: :exclude
  :inline_function_patterns:
    - 'static inline'
    - 'static'
  :mock_prefix: mock_
  :when_no_prototypes: :warn
  :enforce_strict_ordering: TRUE
  :plugins:
    - :ignore
    - :callback
  :treat_as:
    uint8:    HEX8
    uint16:   HEX16
    uint32:   UINT32
    int8:     INT8
    bool:     UINT8

CMock correctly handles the following header file:

#ifndef HEADER_H
#define HEADER_H

static inline void func2(int x,
                         int y);

#endif

It does not mock the func2() function as expected.

But the following header file does not get handler correctly:

#ifndef HEADER_H
#define HEADER_H

static void func2(int x,
                  int y);

#endif

In this case, the func2() function is being mocked despite the :strippables and :inline_function_patterns. Note that I've over-configured with additional patterns just to be overly sure that static void func2() will be ignored by CMock.

I'll also note that I've tried it with Ceedling with both :use_test_preprocessor: TRUE and :use_test_preprocessor: FALSE.

mvandervoord commented 1 year ago

I believe strippables happen before the inline processing, so in your instance, you're telling it to remove all the static keywords before it analyzes each line. So they shouldn't be a factor by the time the line is analyzed.

What's the purpose of the second strippable? You're ending with .* which matches everything?

In any case, once the static is removed, the function void func2(int x, int y) is a valid function that should be mocked.

JonTBeckett commented 1 year ago

Modifying the config to

:cmock:
  :treat_inlines: :exclude
  :inline_function_patterns:
    - 'static inline'
    - 'static'
  :mock_prefix: mock_
  :when_no_prototypes: :warn
  :enforce_strict_ordering: TRUE
  :plugins:
    - :ignore
    - :callback
  :treat_as:
    uint8:    HEX8
    uint16:   HEX16
    uint32:   UINT32
    int8:     INT8
    bool:     UINT8

doesn't change the result. It still incorrectly mocks the func2() function.

As for the the second strippable, that regex should match the entire line that starts with "static". The goal there was to wipe out the entire line even though the declaration spans two lines. I'm honestly just shooting in the dark trying a mass amount of varying configurations trying to figure out what is going on. What is clear to me though, is that CMock is mocking functions in header files that have declarations that begin with "static".

What would the recommended configuration be so that when mocking the following header, func2() is NOT mocked?

#ifndef HEADER_H
#define HEADER_H

static void func2(int x, int y);

#endif

Because none of the configurations I've listed anywhere in the comments of this issue have managed to accomplish that. The default configuration does not accomplish it either. It is my understanding that the :treat_inline configuration should ignore static functions by default and that my :inline_function_patterns configuration should make it clear that static functions should be ignored by CMock. Is this an incorrect understanding of the summary markdown?

mvandervoord commented 1 year ago

At some point in this conversation, you've changed focus from wanting to understand why an inline function was being mocked (the answer being that there were non-inlined declarations) to wanting CMock to ignore static functions.

The inline-detection mechanism is much more than just pattern matching (see how inline_function_regex_formats is used if you're curious). Much of that complexity is to keep it from getting confused with cases like yours, where people are mixing static and inline. A static function is not necessarily an inline, and therefore shouldn't be treated like one.

Am I understanding correctly that you'd like a feature where we can tell CMock to not mock all static functions? What kind of situation does this solve? I'd want to add test cases to verify it's working properly.

JonTBeckett commented 1 year ago

So I think that the summary markdown is what is confusing me here. Because it lumps static functions in with the treat inline functionality. See the quote from the CMock_Summary.md below:

This has lead me to believe that the treat inlines functionality will also cause CMock to ignore functions declared static in a header file. If this is an incorrect understanding of the above quote from the CMock_Summary.md, then I certainly am barking up the wrong tree. If I'm misunderstanding how the treat_inlines functionality works then I will close this issue and look for an alternative way to configure Cmock to not mock the static functions.

Could you clarify the above CMock_Summar.md quote for me?

mvandervoord commented 1 year ago

It appears this was a conflict of features. The original submitter of the inline feature seems to have submitted support for stripping static functions as well. It conflicted with most people's desire to treat static like any other function and just mock it. The latter case was fixed, leaving the first case broken (but still documented).

I don't mind fixing this in a way that can support both... I'm just wondering what the usecase is. What's a situation where you would want to ignore static functions and not mock them? What makes a static function different than any other function in a header file?

(sorry about the delayed response, I was digging through the history to get the answer.)

JonTBeckett commented 1 year ago

Alright, that clears up my confusion then. That piece of the summary markdown was the only reason I was expecting the static function be ignore like an inline function from CMock's point of view. I will go ahead and close this as an issue since it isn't a bug in CMock.

As for the use case of mocking static functions, it would only be this specific case to allow me to mock the crypto.h header without the manual effort of making a trimmed down duplicate just for mocking. I got excited when I saw that the treat inline feature would cause CMock to not mock the static functions which would eliminate the redefinition error. I would simply use the unmocked inline version of those functions which is just fine for my tests.

At any rate, you've answered my questions and convinced me this isn't a bug, just a documentation detail. Thanks for the support! I'll look into other options for avoiding the duplicate definition error (without the manual copy/modify step) since it is a wonky architecture that I'm stuck with.

mvandervoord commented 1 year ago

Thanks Jon.

In the long run, the best answer is we want to be able to mock some functions and not others from header files (this is true if they are static or not). There are times when API's redefine a function prototype in multiple headers and the current system mocks both... so if you need to include both of those files, you run into a problem similar to yours.

This is a feature that is being worked on, but isn't slated to be in the next release yet, unfortunately. Fingers crossed that it's soon. ;)