This is an archive of the discontinued LLVM Phabricator instance.

Removed platform-specific ifdefs from sanitizer_procmaps.h
ClosedPublic

Authored by Vurchin on Sep 20 2017, 1:51 PM.

Event Timeline

Vurchin created this revision.Sep 20 2017, 1:51 PM

title/description are misleading. I don't see anything related to ifdefs

Why do you need to change this, it seems right now it's were it should be.

lib/sanitizer_common/sanitizer_procmaps_freebsd.cc
18

This is linux file. It can contain, even in future, things incompatible with FREEBSD

vitalybuka accepted this revision.Sep 20 2017, 2:11 PM
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_procmaps_freebsd.cc
18

Meh, it's called linux, but already has SANITIZER_FREEBSD || SANITIZER_LINUX || SANITIZER_NETBSD

This revision is now accepted and ready to land.Sep 20 2017, 2:11 PM
fjricci requested changes to this revision.Sep 20 2017, 2:16 PM

We're working on getting some git issues sorted out, this isn't currently the full commit

This revision now requires changes to proceed.Sep 20 2017, 2:16 PM
Vurchin updated this revision to Diff 116079.Sep 20 2017, 2:44 PM
Vurchin edited edge metadata.

arc again

vitalybuka added inline comments.Sep 20 2017, 3:01 PM
lib/sanitizer_common/sanitizer_procmaps.h
82

Instead of dynamic allocation maybe just:

#inlude <sanitizer_procmaps_linux.h>
#inlude <sanitizer_procmaps_mac.h>
... 
MemoryMappingLayoutData data_;
lib/sanitizer_common/sanitizer_procmaps_linux.cc
21

missalingned

Vurchin planned changes to this revision.Sep 20 2017, 3:03 PM

having trouble with git

not finished

Vurchin updated this revision to Diff 116093.Sep 20 2017, 3:23 PM

updated with a new version

Vurchin added inline comments.Sep 20 2017, 4:03 PM
lib/sanitizer_common/sanitizer_procmaps.h
82

So do you think I should create these two files?

fjricci added inline comments.Sep 20 2017, 4:05 PM
lib/sanitizer_common/sanitizer_procmaps.h
82

I would lean towards just using sanitizer_mac.h, like you already did for sanitizer_linux.h, since it seems silly to create a header for just one struct. But will defer to @vitalybuka

Vurchin added inline comments.Sep 20 2017, 4:06 PM
lib/sanitizer_common/sanitizer_procmaps.h
82

Haha, alright, I also like that idea

vitalybuka added inline comments.Sep 20 2017, 8:02 PM
lib/sanitizer_common/sanitizer_procmaps.h
82

sanitizer_mac.h and sanitizer_linux.h are good as well

I will test this on NetBSD after Monday.

Vurchin updated this revision to Diff 116216.Sep 21 2017, 10:30 AM

Addressed vitaly's comments

Vurchin updated this revision to Diff 116222.Sep 21 2017, 10:49 AM

deleted repeatedly included sanitizer_mac.h

LGTM with few nits

lib/sanitizer_common/sanitizer_linux.h
34

please fix formating,
e.g with git clang-format --style file -f HEAD~1

lib/sanitizer_common/sanitizer_procmaps.h
19

order please

23

you don'e need this forward declaration as it's already included just above.

lib/sanitizer_common/sanitizer_procmaps_common.cc
20

unneeded include?

lib/sanitizer_common/sanitizer_procmaps_freebsd.cc
16

unneeded include?

lib/sanitizer_common/sanitizer_procmaps_linux.cc
17

already included with sanitizer_procmaps.h

lib/sanitizer_common/sanitizer_procmaps_mac.cc
48

please don't make such changes unrelated code.

200

argument "MemoryMappedSegmentData *seg_data" is not needed

207–208

no point to CHECK here

@krytarowski - how important is it for you to pre-test this on NetBSD? Does NetBSD run on buildbots? Just asking because Tuesday is a ways off, and this change seems fairly unlikely to break things given that it passes darwin/linux tests locally and is more or less NFC.

Vurchin updated this revision to Diff 116239.Sep 21 2017, 11:49 AM

address vitaly's comments

Vurchin updated this revision to Diff 116242.Sep 21 2017, 12:07 PM

adding whitespaces back

Vurchin updated this revision to Diff 116248.Sep 21 2017, 12:51 PM

putting back *seg_data

I think the argument *seg_data in NextSegmentLoad is still necessary, since it is a private member and the static function has no access to it

vitalybuka accepted this revision.Sep 21 2017, 12:59 PM
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_procmaps.h
23

what about this one?

Vurchin updated this revision to Diff 116254.Sep 21 2017, 1:09 PM

Removed forward declaration

fjricci accepted this revision.Sep 22 2017, 8:12 AM

LGTM with one nit.

lib/sanitizer_common/sanitizer_procmaps_linux.cc
16

This shouldn't be needed anymore.

This revision is now accepted and ready to land.Sep 22 2017, 8:12 AM
Vurchin updated this revision to Diff 116370.Sep 22 2017, 10:26 AM

removed unneeded header

This revision was automatically updated to reflect the committed changes.