This is an archive of the discontinued LLVM Phabricator instance.

Differential D35553

Define OFF_T as 64-bit integer on NetBSD
ClosedPublic

Authored by krytarowski on Jul 18 2017, 5:22 AM.

Details

Reviewers
joerg
filcab
kcc
vitalybuka
Commits
rGe528bd219343: Define OFF_T as 64-bit integer on NetBSD
rCRT310349: Define OFF_T as 64-bit integer on NetBSD
rL310349: Define OFF_T as 64-bit integer on NetBSD
Summary

All 32 and 64 bit NetBSD platforms define off_t as 64-bit integer.

Part of the code inspired by the original work on libsanitizer in GCC 5.4 by Christos Zoulas.

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Jul 18 2017, 5:22 AM
Herald added a subscriber: kubamracek. · View Herald TranscriptJul 18 2017, 5:22 AM
emaste added a subscriber: emaste.Jul 18 2017, 5:29 AM
emaste added inline comments.
lib/sanitizer_common/sanitizer_internal_defs.h
131–132

This comment needs to be updated or removed.

joerg edited edge metadata.Jul 18 2017, 6:00 AM

This doesn't make sense to me, both the existing and new code. off_t is a signed type.

krytarowski updated this revision to Diff 107306.Jul 19 2017, 8:18 AM

Update comment

krytarowski retitled this revision from Define OFF_T as 64-bit unsigned integer on NetBSD to Define OFF_T as 64-bit integer on NetBSD.Jul 19 2017, 8:19 AM
krytarowski edited the summary of this revision. (Show Details)
krytarowski added inline comments.
lib/sanitizer_common/sanitizer_internal_defs.h
131–132

I've updated it and removed details. I assume that people are well aware about off64_t on Linux.

filcab added inline comments.Jul 19 2017, 9:40 PM
lib/sanitizer_common/sanitizer_internal_defs.h
135

I don't think rewording the comment should be done in the same commit as adding NetBSD support.

krytarowski added inline comments.Jul 20 2017, 6:35 AM
lib/sanitizer_common/sanitizer_internal_defs.h
135

I can add a note that NetBSD is special too. Is this fine?

filcab edited edge metadata.Jul 20 2017, 9:11 AM

Thank you,
Filipe

filcab added inline comments.Jul 20 2017, 10:24 AM
lib/sanitizer_common/sanitizer_internal_defs.h
135

Forgot to comment: Yes, it's fine. Feel free to reflow the comment if needed after your addition. If it's directly related to the patch, it makes sense to change the comment.

krytarowski updated this revision to Diff 109722.Aug 4 2017, 5:55 AM

Update comment

emaste added inline comments.Aug 4 2017, 7:30 AM
lib/sanitizer_common/sanitizer_internal_defs.h
131–132

@filcab the list of OSes in the comment exactly repeats the following #if and adds no value, is there any reason we should repeat them in the comment?

vitalybuka mentioned this in D36314: Add NetBSD support in sanitizer_unwind_linux_libcdep.cc.Aug 4 2017, 5:32 PM
krytarowski added a comment.Aug 7 2017, 4:01 AM

ping?

vitalybuka accepted this revision.Aug 7 2017, 6:16 PM

Please fix the comment

This revision is now accepted and ready to land.Aug 7 2017, 6:16 PM
krytarowski added a comment.Aug 8 2017, 4:21 AM
In D35553#834682, @vitalybuka wrote:

Please fix the comment

What's the desired comment? I will improve it on demand after committing this change.

krytarowski closed this revision.Aug 8 2017, 4:41 AM
vitalybuka added inline comments.Aug 8 2017, 11:35 AM
lib/sanitizer_common/sanitizer_internal_defs.h
131–132

about redundant oses list

Revision Contents

PathSize
lib/
sanitizer_common/
7 lines

Diff 107306

lib/sanitizer_common/sanitizer_internal_defs.h

Show First 20 Lines • Show All 122 Lines • ▼ Show 20 Lines
// Use void* to avoid including <windows.h> everywhere. // Use void* to avoid including <windows.h> everywhere.
typedef void* fd_t; typedef void* fd_t;
typedef unsigned error_t; typedef unsigned error_t;
#else #else
typedef int fd_t; typedef int fd_t;
typedef int error_t; typedef int error_t;
#endif #endif
typedef int pid_t; typedef int pid_t;
// WARNING: OFF_T may be different from OS type off_t, depending on the value of // WARNING: OFF_T may be different from OS type off_t
emasteUnsubmitted
Not Done
ReplyInline Actions

This comment needs to be updated or removed.

emaste: This comment needs to be updated or removed.
krytarowskiAuthorUnsubmitted
Not Done
ReplyInline Actions

I've updated it and removed details. I assume that people are well aware about off64_t on Linux.

krytarowski: I've updated it and removed details. I assume that people are well aware about off64_t on Linux.
emasteUnsubmitted
Not Done
ReplyInline Actions

@filcab the list of OSes in the comment exactly repeats the following #if and adds no value, is there any reason we should repeat them in the comment?

emaste: @filcab the list of OSes in the comment exactly repeats the following `#if` and adds no value…
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

about redundant oses list

vitalybuka: about redundant oses list
// _FILE_OFFSET_BITS. This definition of OFF_T matches the ABI of system calls #if SANITIZER_FREEBSD || SANITIZER_MAC || SANITIZER_NETBSD || \
// like pread and mmap, as opposed to pread64 and mmap64.
// FreeBSD, Mac and Linux/x86-64 are special.
filcabUnsubmitted
Not Done
ReplyInline Actions

I don't think rewording the comment should be done in the same commit as adding NetBSD support.

filcab: I don't think rewording the comment should be done in the same commit as adding NetBSD support.
krytarowskiAuthorUnsubmitted
Not Done
ReplyInline Actions

I can add a note that NetBSD is special too. Is this fine?

krytarowski: I can add a note that NetBSD is special too. Is this fine?
filcabUnsubmitted
Not Done
ReplyInline Actions

Forgot to comment: Yes, it's fine. Feel free to reflow the comment if needed after your addition. If it's directly related to the patch, it makes sense to change the comment.

filcab: Forgot to comment: Yes, it's fine. Feel free to reflow the comment if needed after your…
#if SANITIZER_FREEBSD || SANITIZER_MAC || \
(SANITIZER_LINUX && defined(__x86_64__)) (SANITIZER_LINUX && defined(__x86_64__))
typedef u64 OFF_T; typedef u64 OFF_T;
#else #else
typedef uptr OFF_T; typedef uptr OFF_T;
#endif #endif
typedef u64 OFF64_T; typedef u64 OFF64_T;
#if (SANITIZER_WORDSIZE == 64) || SANITIZER_MAC #if (SANITIZER_WORDSIZE == 64) || SANITIZER_MAC
▲ Show 20 Lines • Show All 250 Lines • Show Last 20 Lines