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.

Diff Detail

Repository
rL LLVM

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
17 ↗(On Diff #116065)

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
17 ↗(On Diff #116065)

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
89 ↗(On Diff #116079)

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 ↗(On Diff #116079)

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
89 ↗(On Diff #116079)

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
89 ↗(On Diff #116079)

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
89 ↗(On Diff #116079)

Haha, alright, I also like that idea

vitalybuka added inline comments.Sep 20 2017, 8:02 PM
lib/sanitizer_common/sanitizer_procmaps.h
89 ↗(On Diff #116079)

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 ↗(On Diff #116222)

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

lib/sanitizer_common/sanitizer_procmaps.h
19 ↗(On Diff #116222)

order please

25 ↗(On Diff #116222)

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

lib/sanitizer_common/sanitizer_procmaps_common.cc
20 ↗(On Diff #116222)

unneeded include?

lib/sanitizer_common/sanitizer_procmaps_freebsd.cc
16 ↗(On Diff #116222)

unneeded include?

lib/sanitizer_common/sanitizer_procmaps_linux.cc
17 ↗(On Diff #116222)

already included with sanitizer_procmaps.h

lib/sanitizer_common/sanitizer_procmaps_mac.cc
48 ↗(On Diff #116222)

please don't make such changes unrelated code.

185 ↗(On Diff #116222)

argument "MemoryMappedSegmentData *seg_data" is not needed

192 ↗(On Diff #116222)

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
25 ↗(On Diff #116222)

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 ↗(On Diff #116254)

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.