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

Can't use %ld to portably print time_t values #1084

Closed
nmeum opened this issue Feb 14, 2021 · 3 comments · Fixed by #1085
Closed

Can't use %ld to portably print time_t values #1084

nmeum opened this issue Feb 14, 2021 · 3 comments · Fixed by #1085

Comments

@nmeum
Copy link

nmeum commented Feb 14, 2021

Printing values of type time_t using the %ld format string assumes a specific time_t representation which may not hold. For example, on musl libc time_t is a 64 bit value, even on 32 bit systems thus time_t is not a long int on 32 bit musl architectures. See the musl time64 Release Notes for more information. Currently, this causes the following compiler warning when attempting to compile tig on a 32 bit musl system:

src/util.c:163:48: warning: format '%ld' expects argument of type 'long int', but argument 6 has type 'time_t' {aka 'long long int'} [-Wformat=]
  163 |    if (!string_nformat(buf, buflen, NULL, "%s%ld%c",
      |                                              ~~^
      |                                                |
      |                                                long int
      |                                              %lld
  164 |         now.tv_sec >= timestamp ? "" : "-",
  165 |         seconds, reldate[i].compact_symbol))
      |         ~~~~~~~                                 
      |         |
      |         time_t {aka long long int}
src/util.c:168:52: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'time_t' {aka 'long long int'} [-Wformat=]
  168 |   } else if (!string_nformat(buf, buflen, NULL, "%ld %s%s %s",
      |                                                  ~~^
      |                                                    |
      |                                                    long int
      |                                                  %lld
  169 |         seconds, reldate[i].name,
      |         ~~~~~~~                                     
      |         |
      |         time_t {aka long long int}

It will likely result in a wrong output on these system as well, didn't test this though.

algitbot pushed a commit to alpinelinux/aports that referenced this issue Feb 14, 2021
@koutcher
Copy link
Collaborator

There is one occurence in src/view.c as well. I noticed you are using PRId64 but it doesn't give the expected result on macOS. PRIdMAX does and I believe it would give the same result as PRId64 with musl.

@nmeum
Copy link
Author

nmeum commented Feb 22, 2021

My patch was just intended as a quick fix for Alpine. I think using time_t values with printf(3) always relies on implementation-defined behaviour. The proper way to address this is likely by using strftime(3).

@koutcher
Copy link
Collaborator

Noted. strftime() is really not convenient here but, as every occurrence is associated to a time delta, we could use difftime(3).

koutcher added a commit to koutcher/tig that referenced this issue Feb 26, 2021
algitbot pushed a commit to alpinelinux/aports that referenced this issue Mar 22, 2021
drop patch, issue solved upstream jonas/tig#1084
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 a pull request may close this issue.

2 participants