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.

Sign up for GitHub

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

Jump to bottom

pkg/libb2: rename config.h to avoid conflicts in the global namespace, set HAVE_ALIGNED_ACCESS_REQUIRED based on CPU (fixes build on esp8266) #12135

Merged
merged 3 commits into from
Sep 4, 2019
Merged

pkg/libb2: rename config.h to avoid conflicts in the global namespace, set HAVE_ALIGNED_ACCESS_REQUIRED based on CPU (fixes build on esp8266) #12135

merged 3 commits into from
Sep 4, 2019

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 31, 2019 •
edited
Loading

Contribution description

tests/pkg_libb2 fails to build on esp8266 targets. The SUFFIX is used to select a CPU specific optimized implementation, e.g. SUFFIX=_avx enables the AVX code path, SUFFIX=_ssse3 builds with SSE3 instructions and so on. Since we do not evaluate src/Makefile.am, SUFFIX is never set and the preprocessor just pasts it literal, resulting in function names like blake2bSUFFIX for which no implementation exists.

To fix this, set SUFFIX to empty string to select the generic implementation.

Testing procedure

Build tests/pkg_libb2

Issues/PRs references

Discovered in #12133

@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 31, 2019
@benpicco benpicco requested a review from gschorcht August 31, 2019 15:17
gschorcht
gschorcht reviewed Aug 31, 2019
View reviewed changes
pkg/libb2/Makefile.libb2 Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor Author

benpicco commented Sep 1, 2019

Ok, I created a patch for the libb2 pkg to rename the config.h file to libb2_config.h. I also added a definition of HAVE_ALIGNED_ACCESS_REQUIRED if the CPU requires it. (taken from GStreamer)

@kaspar030
Copy link
Contributor

I created a patch for the libb2 pkg to rename the config.h file to libb2_config.h.

+1. Somehow using "config.h" in the global header namespace seems like a poor choice from the start...

@benpicco benpicco changed the title pkg/libb2: fix build on esp8266 pkg/libb2: rename config.h to avoid conflicts in the global namespace, set Sep 2, 2019
@benpicco benpicco changed the title pkg/libb2: rename config.h to avoid conflicts in the global namespace, set pkg/libb2: rename config.h to avoid conflicts in the global namespace, set HAVE_ALIGNED_ACCESS_REQUIRED based on CPU (fixes build on esp8266) Sep 2, 2019
@benpicco
Copy link
Contributor Author

benpicco commented Sep 2, 2019

On a second thought, adding a define to native and then relying on that to detect if we can do unaligned access or not seems like the cleaner solution.

If this stretches the scope of this PR too much, I can remove it.

@gschorcht
Copy link
Contributor

Works on: arduino-due (arm), esp8266, esp32, iotlab-m3, native.
Craches on: arduino-mega2560 (avr)

@benpicco
Copy link
Contributor Author

benpicco commented Sep 2, 2019

Huh that's weird - does it also crash on master?

@gschorcht
Copy link
Contributor

gschorcht commented Sep 2, 2019 •
edited
Loading

Huh that's weird - does it also crash on master?

It seems so. At least the output

main(): This is RIOT! (Version: 2019.10-devel-556-g93fa4)
.
blake2_tests.test_blake2s (tests/pkg_libb2/main.c 56) memcmp(b2s, hash, sizeof hash) == 0
.␌

is repeated three times. I guess it crashes and restarts three times before the output stops. With the PR, the output above is repeated in an endless loop.

@gschorcht
Copy link
Contributor

gschorcht commented Sep 4, 2019 •
edited
Loading

@benpicco Please squash.

I tried to investigate the problem a bit more. blake2s returns a wrong hash value on AVR independent on whether HAVE_ALIGNED_ACCESS_REQUIRED is defined or not. Since it returns the same wrong value for current master, the problem is not related to this PR. Therefore, I would like to merge this PR and open a separate issue (#12168).

benpicco and others added 3 commits September 4, 2019 14:15
@benpicco
cpu/native: advertise support for unaligned memory access
7047a20 
On Linux, even if the architecture does not support it, the kernel will
catch the fault and emulate the unaligned accesss.
@benpicco
pkg/libb2: define HAVE_ALIGNED_ACCESS_REQUIRED based on CPU
bf410c8 
Most architectures do not support unaligned memory access, so set the define accordingly.
@benpicco
pkg/libb2: rename config.h to libb2_config.h
4707f6b 
This is to avoid conflics with other config.h files, e.g. when building
for esp8266 where $(NEWLIB)/xtensa-lx106-elf/include/config.h gets
included instead.
@benpicco
Copy link
Contributor Author

benpicco commented Sep 4, 2019

Squashed.

@gschorcht gschorcht mentioned this pull request Sep 4, 2019
Closed
@benpicco benpicco requested a review from gschorcht September 4, 2019 12:34
@gschorcht
Copy link
Contributor

Lets go.

@gschorcht gschorcht merged commit 98dfdef into RIOT-OS:master Sep 4, 2019
@benpicco benpicco deleted the libb2-fix branch September 4, 2019 12:39
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
@ant9000 ant9000 mentioned this pull request Oct 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gschorcht gschorcht gschorcht approved these changes

Assignees
No one assigned
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
4 participants
@benpicco @kaspar030 @gschorcht @kb2ma