Skip to content

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
gschorcht merged 3 commits into
RIOT-OS:masterfrom
benpicco:libb2-fix
Sep 4, 2019
Merged

Conversation

@benpicco

@benpicco benpicco commented Aug 31, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread pkg/libb2/Makefile.libb2 Outdated
@benpicco

benpicco commented Sep 1, 2019

Copy link
Copy Markdown
Contributor Author

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

kaspar030 commented Sep 2, 2019

Copy link
Copy Markdown
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...

gschorcht
gschorcht reviewed Sep 2, 2019
View reviewed changes
Comment thread pkg/libb2/include/libb2_config.h
gschorcht
gschorcht requested changes Sep 2, 2019
View reviewed changes
Comment thread pkg/libb2/patches/0001-rename-config.h-to-libb2_config.h.patch
@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

benpicco commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

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

gschorcht commented Sep 2, 2019

Copy link
Copy Markdown
Contributor

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

@benpicco

benpicco commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

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

@gschorcht

gschorcht commented Sep 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

gschorcht commented Sep 4, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@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

benpicco commented Sep 4, 2019

Copy link
Copy Markdown
Contributor Author

Squashed.

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

gschorcht commented Sep 4, 2019

Copy link
Copy Markdown
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