This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt santizer] Use clock_gettime instead of timespec_get
ClosedPublic

Authored by jeroen.dobbelaere on Nov 18 2020, 12:32 AM.

Details

Summary

On RH66, timespec_get is not available. Use clock_gettime instead.

This problem was introduced with D87120

Diff Detail

Event Timeline

jeroen.dobbelaere requested review of this revision.Nov 18 2020, 12:32 AM
jeroen.dobbelaere retitled this revision from [compiler-rt santizes] Use clock_gettime as fallback when timespec_get is not available to [compiler-rt santizer] Use clock_gettime as fallback when timespec_get is not available.
vitalybuka added inline comments.Nov 18 2020, 1:11 AM
compiler-rt/lib/memprof/memprof_allocator.cpp
57

maybe we can keep only clock_gettime(CLOCK_REALTIME, &ts); ?

compiler-rt/lib/memprof/memprof_allocator.cpp
57

As far as I see, clock_gettime is POSIX, where timespec_get is C11. You might as wel get in a situation where timespec_get is available, but clock_gettime is not ?

Besides that, I don't really have a preference. If you think that it makes sense to settle on clock_gettime, I can create a patch doing just that. Maybe @tejohnson can elaborate on the choice of timespec_get ?

tejohnson added inline comments.Nov 18 2020, 3:35 PM
compiler-rt/lib/memprof/memprof_allocator.cpp
57

I selected timespec_get since I saw it used in some other code I was looking at, but since it looks like clock_gettime is more portable, let's just switch to that. In fact, looks like glibc implements the former in terms of the latter.

jeroen.dobbelaere retitled this revision from [compiler-rt santizer] Use clock_gettime as fallback when timespec_get is not available to [compiler-rt santizer] Use clock_gettime instead of timespec_get.
jeroen.dobbelaere edited the summary of this revision. (Show Details)

Always use clock_gettime.

jeroen.dobbelaere marked 2 inline comments as done.Dec 3 2020, 6:29 AM
This revision is now accepted and ready to land.Dec 3 2020, 7:46 AM

Could you commit this for me ? I don't have write access. Thanks !

Could you commit this for me ? I don't have write access. Thanks !

I'm having some new issues committing at the moment that I can't figure out (just emailed llvm-dev). @vitalybuka can you commit this?

I'm having some new issues committing at the moment that I can't figure out (just emailed llvm-dev). @vitalybuka can you commit this?

Maybe they are caused by the change in the branch names ('master' -> 'main') ?

I'm having some new issues committing at the moment that I can't figure out (just emailed llvm-dev). @vitalybuka can you commit this?

Maybe they are caused by the change in the branch names ('master' -> 'main') ?

Yep, just figured that out after I sent the llvm-dev email. I can push now. I'll push your change in a little bit.

Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2020, 10:10 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript