Skip to content

minimal-libc: Add define guard around struct timespec #17768

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

Conversation

PiotrZierhoffer
Copy link
Collaborator

This allows compilation with the CONFIG_POSIX_API=y.

There was a regression introduced in
#17468
that caused struct timespec to be redefined.

I reused the same mechanism that was there to prevent
redefinition with NEWLIB.

Signed-off-by: Piotr Zierhoffer [email protected]

@zephyrbot zephyrbot added the area: C Library C Standard Library label Jul 25, 2019
@carlescufi carlescufi requested a review from pabigot July 25, 2019 09:52
@tgorochowik tgorochowik requested a review from jukkar July 25, 2019 09:54
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

As I noted in slack the definition in <posix/time.h> is wrong:

The <time.h> header shall declare the structure timespec, which has at least the following members:
time_t tv_sec Seconds.
long tv_nsec Nanoseconds.

where Zephyr's POSIX header has:

#ifndef __timespec_defined
#define __timespec_defined
struct timespec {
        signed int tv_sec;
        signed int tv_nsec;
};
#endif

time_t in Zephyr minimal libc and most libcs is int64_t.

The approach in this PR will create inconsistencies depending on which header is included first.

The proper fix at this time appears to be to remove the conflicting definition from <posix/time.h>, and have that header include <time.h> before the definition of itimerspec. Why is that not satisfactory?

@pabigot pabigot self-requested a review July 25, 2019 11:11
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

As I noted in slack the definition in <posix/time.h> is wrong:

The <time.h> header shall declare the structure timespec, which has at least the following members:
time_t tv_sec Seconds.
long tv_nsec Nanoseconds.

where Zephyr's POSIX header has:

#ifndef __timespec_defined
#define __timespec_defined
struct timespec {
        signed int tv_sec;
        signed int tv_nsec;
};
#endif

time_t in Zephyr minimal libc and most libcs is int64_t.

The approach in this PR will create inconsistencies depending on which header is included first.

The proper fix at this time appears to be to remove the conflicting definition from <posix/time.h>, and have that header include <time.h> before the definition of itimerspec. Why is that not satisfactory?

@PiotrZierhoffer
Copy link
Collaborator Author

That was my first try, just as you suggested - but I had some troubles verifying it with newlib. As the newlib definition is identical to the one you added to minimal libc I assumed it will be easier to justify the change when it's limited to a single part of code.

I will introduce the change as you suggest and verify it with shippable then.

It collides both with newlib and minimal libc.

Signed-off-by: Piotr Zierhoffer <[email protected]>
@PiotrZierhoffer PiotrZierhoffer force-pushed the timespec_redefinition_fix branch from 37b3d3b to b2246d0 Compare July 25, 2019 11:33
@PiotrZierhoffer PiotrZierhoffer requested a review from pfalcon as a code owner July 25, 2019 11:33
@zephyrbot zephyrbot added the area: API Changes to public APIs label Jul 25, 2019
Copy link
Collaborator

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

This conflicts with #16626

@PiotrZierhoffer
Copy link
Collaborator Author

This conflicts with #16626

Indeed, I didn't notice that. #16626 also conflicts with the current master, as timespec was introduced in #17468

signed int tv_nsec;
};
#endif
#include <time.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this maybe including itself because . is in the include search path?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually the shippable shows -I../../../../../../include/posix is in the search path, which is flat out wrong, unless that's how the posix subsystem was designed to be used, in which case there's good reason to step back and rethink it.

@pfalcon
Copy link
Collaborator

pfalcon commented Jul 25, 2019

Indeed, I didn't notice that. #16626 also conflicts with the current master, as timespec was introduced in #17468

There're no git textual conflicts, but indeed, that introduces another (re)definition point for struct timespec. I'm fixing that in #16626.

@PiotrZierhoffer
Copy link
Collaborator Author

As this PR does not make sense in the light of #16626, I will close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: C Library C Standard Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants