This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer][fuchsia] Avoid deprecated syscall.
ClosedPublic

Authored by jsankey on Oct 26 2020, 9:57 AM.

Details

Summary

The zx_clock_get syscall on Fuchsia is deprecated - ref
https://fuchsia.dev/fuchsia-src/reference/syscalls/clock_get
This changes to the recommended replacement; calling zx_clock_read on
the userspace UTC clock.

Diff Detail

Event Timeline

jsankey created this revision.Oct 26 2020, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2020, 9:57 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
jsankey requested review of this revision.Oct 26 2020, 9:57 AM
jsankey updated this revision to Diff 300794.Oct 26 2020, 2:32 PM

Fixed linter warning.

This change is addressing fxbug.dev/61726 on the Fuchsia side. Note that this is my first time attempting to make an LLVM change; I've verified this compiles by building the Fuchsia toolchain but thats all.

phosek accepted this revision.Oct 26 2020, 11:38 PM

LGTM do you want me to land this change for you?

This revision is now accepted and ready to land.Oct 26 2020, 11:38 PM

LGTM do you want me to land this change for you?

Yes please in a little bit. There isn't any urgency here so I'm happy to leave open for a day or two in case anyone else has comments. In particular I'd like John to take a quick look, I can follow up offline when thats happened.

mcgrathr accepted this revision.Oct 27 2020, 12:26 PM

This is fine. If we wanted to avoid wider use of _zx_utc_reference_get, I think it would also be fine for this one call to use the C11 timespec_get API rather than a pure Zircon API.

Generally, this LGTM. The check against ZX_HANDLE_INVALID is probably redundant. zx_clock_read will reject the call if the handle is invalid, returning an error (which would be caught on line 55) or terminating the calling process (depending on process's policy). I'm fine with leaving the check in place if you want, however.

I do have one quick question, however. Is the implication of how this function works that there must be a UTC clock available to any process which runs with the sanitizer? IOW - what happens if (someday) we end up not granting access to UTC, and someone attempts to run their code with the sanitizer on?

This is probably not a great place to have this discussion, be let's talk about it some offline if you have some time (pun).

I think NanoTime is actually not reached at all. We can probably remove it.

I think NanoTime is actually not reached at all. We can probably remove it.

Even better!

I think NanoTime is actually not reached at all. We can probably remove it.

I did see a couple of places where it looked like NanoTime could be used as a fallback (e.g. scudo_allocator.cpp:280 if the GetRandom fails). Getting rid of NanoTime does sound possible and beneficial longer term but are you OK if this CL just removes the _zx_clock_get reference for now?

I do have one quick question, however. Is the implication of how this function works that there must be a UTC clock available to any process which runs with the sanitizer? IOW - what happens if (someday) we end up not granting access to UTC, and someone attempts to run their code with the sanitizer on?

This is probably not a great place to have this discussion, be let's talk about it some offline if you have some time (pun).

Yes it is the implication that there must be a UTC clock available for processes that end up invoking this function. As Roland points out though there are very few places this is invoked, and at the moment our intent is to supply the UTC clock to all processes to match the global availability of the kernel UTC clock. Getting rid of the function entirely does seem like a good idea long term.

I already approved this as is. I just think it's dead code.

LGTM do you want me to land this change for you?

I think the discussion has finished up now, so yes if you could land this for me I'd really appreciate it.

This revision was automatically updated to reflect the committed changes.