Page MenuHomePhabricator

Segregate FreeBSD-specific parts from sanitizer_procmaps_linux.cc
ClosedPublic

Authored by kutuzov.viktor.84 on Jul 17 2014, 3:39 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kutuzov.viktor.84 retitled this revision from to Segregate FreeBSD-specific parts from sanitizer_procmaps_linux.cc.
kutuzov.viktor.84 updated this object.
kutuzov.viktor.84 edited the test plan for this revision. (Show Details)
kutuzov.viktor.84 added reviewers: kcc, samsonov.
kutuzov.viktor.84 added a subscriber: Unknown Object (MLST).
samsonov added inline comments.Jul 17 2014, 1:41 PM
lib/sanitizer_common/sanitizer_procmaps_common.cc
30 ↗(On Diff #11564)

Can you generalize this with ParseHex() function?

lib/sanitizer_common/sanitizer_procmaps_linux.cc
21 ↗(On Diff #11564)

Shouldn't these vars be deleted from this file?

emaste added a subscriber: emaste.Jul 18 2014, 7:20 AM

ReadHex() rewritten via ParseHex(); extra definitions for cached_proc_self_maps_ and cache_lock_ removed from the Linux-specific source file.

samsonov edited edge metadata.Jul 24 2014, 11:18 AM

I think it's better to first remove ReadHex/ReadDecimal in favor of ParseHex/ParseDecimal in a small separate change, and then proceed with this change.

lib/sanitizer_common/sanitizer_procmaps_common.cc
38 ↗(On Diff #11839)

This is weird you don't have (c >= 'A' && c <= 'F'), although these values are handled in ParseHex().

164 ↗(On Diff #11839)

I believe you can safely replace ReadHex with ParseHex here, and get rid of the former.

169 ↗(On Diff #11839)

... And replace this with ParseDecimal() for consistency.

lib/sanitizer_common/sanitizer_procmaps_linux.cc
90 ↗(On Diff #11839)

Fix comment near this #endif

That is, it's fine to submit the small piece related to ReadHex/ReadDecimal w/o review.

kutuzov.viktor.84 edited edge metadata.

Fixed as suggested; an alternative approach to parsing numbers proposed.

LGTM modulo nits below.

lib/sanitizer_common/sanitizer_procmaps.h
90 ↗(On Diff #11877)

This function is implemented only on FreeBSD and Linux, but not on Mac. Put it near the ProcSelfMapsBuff definition above.

lib/sanitizer_common/sanitizer_procmaps_common.cc
29 ↗(On Diff #11877)

please change it back to c - 'a' + 10

31 ↗(On Diff #11877)

and c - 'A' + 10

32 ↗(On Diff #11877)

remove the parens

samsonov accepted this revision.Jul 25 2014, 11:19 AM
samsonov edited edge metadata.
This revision is now accepted and ready to land.Jul 25 2014, 11:19 AM
kutuzov.viktor.84 updated this revision to Diff 12228.

Closed by commit rL214955 (authored by vkutuzov).