Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add freestanding to all and test of tests/Makefile #1198

Closed
wants to merge 1 commit into from

Conversation

t-mat
Copy link
Contributor

@t-mat t-mat commented Dec 3, 2022

Follow up of #1184 and #1187

Now tests/Makefile contains

  • freestanding as a member of all.
  • test-freestanding as a member of test.

Note that as for all32 (-m32) builds, test-freestanding skips its actual test.


cd /path/to/lz4
cd tests

make all
# cc -ffreestanding -nostdlib freestanding.c -o freestanding

make test
#  ---- test freestanding ----
# ./freestanding
# strace ./freestanding
# ...
# +++ exited with 0 +++
# ltrace ./freestanding
# +++ exited (status 0) +++

@t-mat t-mat force-pushed the add-test-freestanding-to-all branch from 59578be to 95d7c29 Compare December 3, 2022 05:11
Follow up of lz4#1184 and lz4#1187

Now `tests/Makefile` contains

- `freestanding` as a member of `all`.
- `test-freestanding` as a member of `test`.

Note that as for `all32` (`-m32`) builds, `test-freestanding` skips its actual test.
@t-mat
Copy link
Contributor Author

t-mat commented Dec 3, 2022

Sorry, I forgot about ubsan32 case. I'll resolve this error later.

@Cyan4973
Copy link
Member

Cyan4973 commented Feb 2, 2023

Note that as for all32 (-m32) builds, test-freestanding skips its actual test.

Could you tell me why freestanding (or just its tests?) is not compatible with 32-bit ?

@t-mat
Copy link
Contributor Author

t-mat commented Feb 2, 2023

-m32 generates i386 code but os_syscall1() uses rax and 64-bits registers (and its own stack frame).

lz4/tests/freestanding.c

Lines 156 to 161 in 1f3adea

static __inline long os_syscall1(long n, long a1) {
register long rax __asm__ ("rax") = n;
register long rdi __asm__ ("rdi") = a1;
__asm__ __volatile__ ("syscall" : "+r"(rax) : "r"(rdi) : "rcx", "r11", "memory");
return rax;
}

Also, due to lack of my experience with freestanding Linux environment, I just avoid to #include almost everything. As a result, the following line causes incompatibility:

#define SYS_exit (60)

Because each platform has its own numbers for syscall

x64 (exit = 60):
https://github.com/torvalds/linux/blob/v4.17/arch/x86/entry/syscalls/syscall_64.tbl#L71

x86 (exit = 1):
https://github.com/torvalds/linux/blob/v4.17/arch/x86/entry/syscalls/syscall_32.tbl#L15

We may need to include <sys/syscall.h> for portability.

@Cyan4973
Copy link
Member

Cyan4973 commented Feb 2, 2023

Thanks for the clear explanation @t-mat !

We may need to include <sys/syscall.h> for portability.

Isn't the idea of including anything going against the very concept of freestanding ?

edit :
OK, sorry, now I understand that these specificities happen in the test program tests/freestanding.c,
not in the lz4 freestanding library.

In which case, of course, the test itself can be specific to certain platforms.

Maybe this could be called out explicitly in the unit name ?

@t-mat
Copy link
Contributor Author

t-mat commented Feb 4, 2023

these specificities happen in the test program tests/freestanding.c, not in the lz4 freestanding library.

Right. If we can successfully run lz4 freestanding library without standard library (libc, etc) but only depends platform's own syscall, we can say it conforms the concept of the freestanding (bare bone C environment).

Maybe this could be called out explicitly in the unit name ?

Yes. It'd be nice to have explicit / specific name (and maybe test) for them.

@Cyan4973
Copy link
Member

Hi @t-mat ,

it seems I completely lost track of this PR.

If I understand correctly, all it does is enable freestanding test automatically as part of make all and make test,
as well as ensuring it's setup properly for 32-bit x86 target.

Is it still valid as is ?
Could it rebased ?
(I see there was a ubsan test failure, presumably due to some external flaky condition)

@t-mat
Copy link
Contributor Author

t-mat commented Oct 20, 2023

Yeah, I also suspect this PR is outdated. I've tried to implement wider support of freestanding test. But it seems it's been failed.

Please close this PR for now.
When I write better test for other platforms/architectures, I'll send another PR.

@Cyan4973 Cyan4973 closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants