Add cross-platform tests for libmetal

[lntaf]

To-be-expanded suite of tests for libmetal using system-agnostic interfaces instead of system specific tests.

There's more tests coming to expand the suite to other interfaces the library provides for completeness.
As much as possible tests isolate their libmetal usage to the filename of the interface they're testing.
When unavoidable, as is the case with init, an extra test is provided testing just that interface to help narrow down any issues.

Known issues:
mutex and shmem testing are pthread based, this will be changed to metal_run in a followup patch.
Interfaces: device, irq, threads, atomics are missing and will also be added later if there's interest in this.

As such this directory doesn't yet tamper with any pre-existing files and uses a simple Makefile for building, after libmetal is built.

[arnopo]

Hello @lntaf,

Thank you for your contribution!

If I understand your objective correctly, you propose adding independent libmetal tests that are separate from the system/machine, by using only the generic API.

To help us understand your work better, it would be helpful if you could share:

• Your final test hierarchy (how you expect to integrate this in the final step).
• Instructions on how to build and run the tests (at least on a Linux PC). Eventually, this would be integrated into the GitHub CI.

As an initial review, if your tests are platform-independent, I would suggest placing them directly at the root level of the test directory, with system and machine directories for specific implementations.

test
├── CMakeLists.txt
├── metal-header-template.c
├── metal-test.c
├── metal-test.h
├── alloc.c
├── assert.c
├── cache.c
├── config.c
├── cpu.c
├── init.c
├── log.c
├── mutex.c
├── shmem.c
├── sleep.c
├── spinlock.c
├── test_all.c
├── test_all.h
├── time.c
└── utilities.c
├── system
├── machine

[lntaf]

Hello,
Yes that is exactly my goal. We needed a testing suite that utilizes the exact same top-level calls, prints and error codes independent of a machine's architecture or OS so it's easier to debug what went wrong. These tests being system independent would also allow someone to implement a generic machine's calls for the APIs provided by libmetal without also having to provide tests for them under the test/system directory. I've so far used the suite on:

Ubuntu 22.04.3 LTS (Linux 5.15.153.1-microsoft-standard-WSL2 x86_64)
Linux 5.10.4-tag- (Sophgo patch kernel, aarch64)
Linux 5.10.4-tag- (Sophgo patch kernel, riscv64)

All of these systems compiled natively using the provided makefile which currently builds as shown:
gcc -g -I <top libmetal dir>/install/usr/local/include <top libmetal dir>/test/system/cross-platform/src/test_all.c -o <top libmetal dir>/test/system/cross-platform/out/test_all.out -L <top libmetal dir>/install/usr/local/lib -lmetal

test_all.h includes all the individual testfiles and a small struct with the name of each one (for printing purposes) as well as storing the return value of each.
test_all.c is just the main testing source file handling all the calls and printing.
We compile test_all.c from src/ -> test_all.out in out/ directory which compiles each individual source file from the header includes.

I'll work on getting you a hierarchy soon because I'm still working on a couple that may be closer to completion so they could be included. When I submit my followup edit with possibly a few more tests and changing the hierarchy should I add a CMake rule that falls under the -DWITH_TESTS flag?

Any other feedback is much appreciated!

[arnopo]

Hello, Yes that is exactly my goal. We needed a testing suite that utilizes the exact same top-level calls, prints and error codes independent of a machine's architecture or OS so it's easier to debug what went wrong. These tests being system independent would also allow someone to implement a generic machine's calls for the APIs provided by libmetal without also having to provide tests for them under the test/system directory. I've so far used the suite on:

Ubuntu 22.04.3 LTS (Linux 5.15.153.1-microsoft-standard-WSL2 x86_64) Linux 5.10.4-tag- (Sophgo patch kernel, aarch64) Linux 5.10.4-tag- (Sophgo patch kernel, riscv64)

All of these systems compiled natively using the provided makefile which currently builds as shown: gcc -g -I <top libmetal dir>/install/usr/local/include <top libmetal dir>/test/system/cross-platform/src/test_all.c -o <top libmetal dir>/test/system/cross-platform/out/test_all.out -L <top libmetal dir>/install/usr/local/lib -lmetal

test_all.h includes all the individual testfiles and a small struct with the name of each one (for printing purposes) as well as storing the return value of each. test_all.c is just the main testing source file handling all the calls and printing. We compile test_all.c from src/ -> test_all.out in out/ directory which compiles each individual source file from the header includes.

I'll work on getting you a hierarchy soon because I'm still working on a couple that may be closer to completion so they could be included. When I submit my followup edit with possibly a few more tests and changing the hierarchy should I add a CMake rule that falls under the -DWITH_TESTS flag?

That seems coherent to me to build with the -DWITH_TESTS flag.

Any other feedback is much appreciated!

Yes , I would also be interested by more feedback especially concerning current AMD implementation:
@tnmysh, @edmooring, could you have a look, please?

[arnopo]

@lntaf It seems you have added a merge commit to your pull request but did not update the code. You may need to push your updates again. 😃

[lntaf]

Indeed @arnopo, it seems syncing my fork to libmetal put an empty commit into this request, I'll push my changes shortly.
So far I've used the logging helpers where possible, changed the logging test as indicated with a loop, stuck to the 100 char limit where applicable and I'll see if anything else is ready for submitting.

[lntaf]

As far as I can tell, my commits cover all the feedback given, let me know if anything else needs tweaking!

[github-actions]

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

[lntaf]

Let me know if there's anything else required!
I addressed the testing functions being declared as extern, compiling the files seperately through test/CMakeLists.txt, any changes to includes that entails as well as any dead code or other general cleanup.
Do note the slight rename from test_all.c (test_all.h completely eliminated) to test-all.c to follow current naming conventions of binaries and pre-existing metal-test source files

[lntaf]

pushed again for compliance and formatting, no extra changes made

[arnopo]

I run it on a Linux PC using following command:

cmake . -WITH_TESTS -Bbuild
make -C build/
./build/test/test-all-metal-static

The result is:

------utilities.c------
metal: warning:   skipped page size 2097152 - invalid args
metal: warning:   skipped page size 1073741824 - invalid args
metal: error:     -10 divided by 3 rounded down is -3 instead of -4 and rounded up is -2 instead of -3
alloc.c       [PASS] 
assert.c      [PASS] 
cache.c       [PASS] 
config.c      [PASS] 
cpu.c         [PASS] 
init.c        [PASS] 
log.c         [PASS] 
mutex.c       [PASS] 
shmem.c       [PASS] 
sleep.c       [PASS] 
spinlock.c    [PASS] 
utilities.c   [FAIL]  (code 8)

In addition of my comments, please, could you :

• fix test utilities issue
• Squash your commits to ease the reviews. we use to fix directly the commits instead of adding fix commit on top of.

[lntaf]

I'll look into the changes you've mentioned @arnopo but regarding the utilities test,
I was gonna raise that problem as a seperate pull request or issue because I don't think the test is wrong.
-10 divided by 3 is ~ -3.334, rounded down it should be -4 and rounded up should be -3. The utility functions are returning -3 and -2 respectively which actually seems like the utilities API has a fundamental flaw with negative numbers.
At the very least this should be a comment in the utilities API that it is unfit for negative division, otherwise it seems the calculation is wrong.

Also regarding squashing commits, would you like me to just squash all the fixes here or go back to whatever commit is responsible for the proposed changes and change it there? Just to confirm.
I could also squash every commit so far together so the main repo only gets a single commit for the whole pull request if that's preferable.

[arnopo]

I'll look into the changes you've mentioned @arnopo but regarding the utilities test, I was gonna raise that problem as a seperate pull request or issue because I don't think the test is wrong. -10 divided by 3 is ~ -3.334, rounded down it should be -4 and rounded up should be -3. The utility functions are returning -3 and -2 respectively which actually seems like the utilities API has a fundamental flaw with negative numbers. At the very least this should be a comment in the utilities API that it is unfit for negative division, otherwise it seems the calculation is wrong.

right After cross check the macros are only support positive values. That show the utility of the test 😃

Also regarding squashing commits, would you like me to just squash all the fixes here or go back to whatever commit is responsible for the proposed changes and change it there? Just to confirm. I could also squash every commit so far together so the main repo only gets a single commit for the whole pull request if that's preferable.

I don't remember the initial split but the idea is to go back to the initial commit split, integrating fixup when possible, else with a dedicated commit (if for instance you rename existing functions).

[nathalie-ckc]

Hi @lntaf , I do program management for the OpenAMP project. No pressure, just making you aware in case you are not on the openamp-rp mailing list: The feature freeze is on Oct 18 for the October release of OpenAMP, if you wish to catch this release. https://lists.openampproject.org/archives/list/openamp-rp@lists.openampproject.org/thread/SO54TZBU4AG7UPHKYLCR6YTVG6YCVMVP/

[lntaf]

Hello @nathalie-ckc , thanks for letting me know! I'll see what I can do about wrapping this up as soon as possible to allow time for any last input before the freeze!

[lntaf]

I believe that's everything, should be ready to merge as far as I can tell and I just squashed every fix commit to my initial commit, seperating out the two commits that modified existing testing api files.

[arnopo]

Seems that there is some merge conflict replicate in you code

[arnopo]

issue in your rebase ?

[lntaf]

Apologies for the delay, I believe everything is in order now

[tnmysh]

@lntaf Thanks for your work.

Following enhancement I suggest, but we don't have to address it immediately.

we can initialize metal log handler common for all the tests.

struct metal_init_params metal_param = METAL_INIT_DEFAULTS;
metal_init(&metal_param);                                                  
metal_set_log_handler(metal_default_log_handler);

This can be common for all the tests. So can be initialized in main, before starting all the tests.
We can choose log level per tests.

@arnopo , are you okay if we merge current patch as it is and do above enhancement after the release?

Thanks.

[arnopo]

@arnopo , are you okay if we merge current patch as it is and do above enhancement after the release?

Thanks.
@tnmysh
Yes it is too late to include this in this release as code is frozen. I plan to integrate it just after the release.

[arnopo]

@lntaf: Tanks for your test suite. I merged them and created #349 issue for the failed test.