define TZ environment variable to match current Intl.DateTimeFormat#16517
define TZ environment variable to match current Intl.DateTimeFormat#16517arsnyder16 wants to merge 17 commits intoemscripten-core:mainfrom
Conversation
system/lib/libc/emscripten_time.c
Outdated
| __attribute__((__weak__)) int daylight = 0; | ||
| __attribute__((__weak__)) char *tzname[2] = { 0, 0 }; | ||
| thread_local char buffer[128]; | ||
| thread_local char timezone_buffer[128]; |
There was a problem hiding this comment.
This can do inside the emscripten_get_timezone function.
There was a problem hiding this comment.
/root/cache/sysroot/include/threads.h:12:22: note: expanded from macro 'thread_local'
#define thread_local _Thread_local
^
/root/project/system/lib/libc/emscripten_time.c:33:10: error: address of stack memory associated with local variable 'buffer' returned [-Werror,-Wreturn-stack-address]
return buffer;
^~~~~~
2 errors generated.
There was a problem hiding this comment.
Maybe you also need static then?
There was a problem hiding this comment.
My understanding was that thread_local implied static, so I'm surprised that you would need it: https://stackoverflow.com/questions/22794382/are-c11-thread-local-variables-automatically-static#:~:text=Thread%20local%20storage%20is%20static,one%20instance%20of%20the%20variable.
There was a problem hiding this comment.
i think it does imply static, but the error is related to it being a "stack" variable which seems to be a little off, is there a reason you prefer static but local function variable vs just thread_local as global?
There was a problem hiding this comment.
threads.h
#ifdef __cplusplus
extern "C" {
typedef unsigned long thrd_t;
#else
typedef struct __pthread *thrd_t;
#define thread_local _Thread_local
#endif
There was a problem hiding this comment.
@sbc100 How do you want me to proceed now?
There was a problem hiding this comment.
Since this is C code you must explicitly add the static specifier. So use static thread_local
There was a problem hiding this comment.
To be clear the error might be a little misleading: variables must have global storage. Here "global storage" also includes function-local statics.
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
LGTM! (after one last comment)
|
I think if you rebase (or merge) the current failures will go away. |
|
Otherwise I think this is ready to land |
system/lib/libc/emscripten_time.c
Outdated
| void _emscripten_get_timezone_js(char* buffer, int length); | ||
|
|
||
| const char* emscripten_get_timezone() { | ||
| static thread_local char buffer[128]; |
There was a problem hiding this comment.
Where does 128 come from? Might be worth a comment.
There was a problem hiding this comment.
good call, looked deeper into the Tz database to properly size, also added a comment
Head branch was pushed to by a user without write access
|
Can you update the PR title and description to match what the change actually does? Or comment here with a proposal for the working on the commit message? |
arsnyder16
changed the title
2 participants
In order to facilitate timezone detection there are a few standard ways of doing that "TZ" environment variable and
etc/localtime.This will allow libraries like icu4c to properly detect the current system timezone, which look for IANA time zone names in a few locations
The TZ environment variable should work out of the box for icu4c
The
etc/localtimesymlink requires icu4c to be built with CHECK_LOCALTIME_LINK which is defined if any of these are true