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

sys/cbor: fix compilation with newlib #7783

Merged
merged 3 commits into from
Oct 24, 2017
Merged

Conversation

kYc0o
Copy link
Contributor

@kYc0o kYc0o commented Oct 20, 2017

Fixes #7411 and maybe #7287.

In some arm-none-eabi-gcc libc strptime in<time.h> is guarded by __GNU_VISIBLE define, which is defined while setting the define _GNU_SOURCE, otherwise strptime is not defined and thus any usage of it fails to compile.

This is the case in cbor, which doesn't include <time.h> explicitly, as well as setting the define _GNU_SOURCE is missing.

This PR fixes that.

@kYc0o kYc0o added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 20, 2017
@kYc0o kYc0o added this to the Release 2017.10 milestone Oct 20, 2017
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could better model this. As a (pseudo-)module (e. g. posix_time) for instance.

@@ -2,6 +2,8 @@ MODULE = cbor

ifneq ($(shell uname -s),Darwin)
CFLAGS += -D_XOPEN_SOURCE=600
else
CFLAGS += -D_GNU_SOURCE=1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why non-Darvin only? Is OSX not using newlib for cross-compiling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is not a OS X related issue, but gcc-arm-none-eabi libc (and maybe also avr-gcc, cannot test because my compiler is broken).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But uname - s returns info about the host system, not the toolchain in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, let me check why it works then.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non darwin guard was to prevent multiple definition: e867d83e
However the test only made sense for native.

I also had the problem with arm-gcc 4.9.4, but not with 6.3.1 anymore.

The new GNU_SOURCE you set is only done for Darwin (it is an ifneq). So it does not fixes linux + arm-gcc 4.9.4.

@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 20, 2017

I think we could better model this. As a (pseudo-)module (e. g. posix_time) for instance.

As far as I understand this is not a POSIX function but a GNU extension, thus, I don't think this should be a POSIX pseudo module.

@miri64
Copy link
Member

miri64 commented Oct 20, 2017

As far as I understand this is not a POSIX function but a GNU extension, thus, I don't think this should be a POSIX pseudo module.

At least the latest POSIX spec defines it

@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 20, 2017

Well, the conclusion of my tests are:

  • In non-Darwin machines (e.g. Linux) _XOPEN_SOURCE must be defined to use strptime.
  • In Darwin _XOPEN_SOURCE is defined by default when building for native.
  • Thus, the previous check disables _XOPEN_SOURCE when building for non-native platforms, however
  • _GNU_SOURCE is the actual define which enables the strptime function in some (new) arm-none-eabi-gcc.

I pushed a fixup which defines _GNU_SOURCE only if cbor_ctime is used.

@jnohlgard
Copy link
Member

Does it work if you define _DEFAULT_SOURCE instead of XOPEN_SOURCE or GNU_SOURCE?

@miri64
Copy link
Member

miri64 commented Oct 23, 2017

@gebart not sure... From the glibc doc (couldn't find anything for newlib with my quick search):

Macro: _DEFAULT_SOURCE
If you define this macro, most features are included apart from X/Open, LFS and GNU extensions: the effect is to enable features from the 2008 edition of POSIX, as well as certain BSD and SVID features without a separate feature test macro to control them. Defining this macro, on its own and without using compiler options such as -ansi or -std=c99, has the same effect as not defining any feature test macros; defining it together with other feature test macros, or when options such as -ansi are used, enables those features even when the other options would otherwise cause them to be disabled.

@miri64
Copy link
Member

miri64 commented Oct 23, 2017

@kYc0o have a look at kYc0o#4 for a cleaner integration into our build system.

@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 24, 2017

Hmm... I suppose I should remove the merge commit is it?

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, please rebase so that the merge commit disappears.

@miri64
Copy link
Member

miri64 commented Oct 24, 2017

(and squash your own ;-))

@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 24, 2017

Squashed and rearranged commits.

@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 24, 2017
@kYc0o
Copy link
Contributor Author

kYc0o commented Oct 24, 2017

Go!

@kYc0o kYc0o changed the title sys/cbor: fix non-defined strptime for some (new) compilers sys/cbor: fix compilation for newlib Oct 24, 2017
@kYc0o kYc0o changed the title sys/cbor: fix compilation for newlib sys/cbor: fix compilation with newlib Oct 24, 2017
@kYc0o kYc0o merged commit a6cb09c into RIOT-OS:master Oct 24, 2017
@kYc0o kYc0o deleted the fix_cbor_arm branch October 24, 2017 14:26
@miri64
Copy link
Member

miri64 commented Oct 24, 2017

Please provide backport ;-)

@kYc0o kYc0o restored the fix_cbor_arm branch October 24, 2017 14:29
@miri64 miri64 removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 24, 2017
@miri64
Copy link
Member

miri64 commented Oct 24, 2017

Backport provided and already merged in #7819

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error (WERROR=1) while compiling the CBOR module
5 participants