1355052 - Tidy up shared-linux-libraries.cc

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Julian Seward [:jseward]]

shared-linux-libraries.cc is a maze of ifdefs which is hard to navigate, hard to
reason about and gets in the way of making a proper fix for bug 1354546.

This bug is for cleanup only.  It should not change any functionality.

It is based on the assumption that MOZ_WIDGET_GONK is only ever relevant when
GP_OS_android is in force, which I think to be true, but I am not sure about.

Comment 1

[Julian Seward [:jseward]]

Attachment: Tidy up shared-linux-libraries.cc

shared-linux-libraries.cc is a maze of ifdefs which is hard to navigate, hard to
reason about and gets in the way of making a proper fix for bug 1354546.  This
bug is for cleanup only.  It should not change any functionality.

The following changes are made:

* adds emacs/vi tab-width lines

* removes the ARRAY_SIZE macro as it appears to be unused

* documents the 3 different configurations, splits #includes accordingly

* comments SharedLibraryInfo::GetInfoForSelf accordingly

* wraps some long lines

* documents in which cases dl_iterate_phdr is used and in which cases
  /proc/<pid>/maps is used

* Puts /proc/<pid>/maps reading in its own scope

* Makes the LOG messages on failure clearer

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8856516 [details] [diff] [review]
Tidy up shared-linux-libraries.cc

Review of attachment 8856516 [details] [diff] [review]:
-----------------------------------------------------------------

Nice clean-up.

At some point we should rename this file shared-libraries-linux-android.cpp, to match platform-linux-android.cpp. Similarly for the Mac and Windows files. Want to do that in a second patch or a follow-up bug?

::: tools/profiler/core/shared-libraries-linux.cc
@@ +23,5 @@
>  #include "common/linux/file_id.h"
>  #include <algorithm>
>  
> +
> +// There are three different configuration cases:

I think it might be even better if we #define constants to give these three cases names. (Like I did with PROFILE_JAVA.) Perhaps USE_ITERATE, USE_ITERATE_AND_MAPS, USE_MAPS?

It might remove the need for the various comments about configs 1, 2 and 3, or let them be shorter.

@@ +50,5 @@
> +# include <sys/types.h>
> +extern "C" MOZ_EXPORT __attribute__((weak))
> +int dl_iterate_phdr(
> +          int (*callback) (struct dl_phdr_info *info,
> +                           size_t size, void *data),

Looks like |size| and |data| would fit on the previous line. And no need for the space between the ')' and '('.

@@ +108,5 @@
>    const char *path = dl_info->dlpi_name;
>  
>    nsAutoString pathStr;
> +  mozilla::Unused
> +    << NS_WARN_IF(NS_FAILED(NS_CopyNativeToUnicode(nsDependentCString(path),

Nit: '<<" on the preceding line.

@@ +157,5 @@
> +    std::string line;
> +    int count = 0;
> +    while (std::getline(maps, line)) {
> +      int ret;
> +      //XXX: needs input sanitizing

I think it's safe to remove this comment. Kernel-controlled input should be trustworthy, and we have a length check, and we have similar code in other places that doesn't sanitize.

@@ +182,5 @@
> +      // since it has no associated phdrs.
> +      if (0 != strcmp(modulePath, "/dev/ashmem/dalvik-jit-code-cache")) {
> +        continue;
> +      }
> +      // Otherwise continue to the tail of the loop, so as to record the entry.

"continue" confused me here, because you're using it in the opposite sense of the |continue| keyword. "proceed" instead?

@@ +196,5 @@
> +#endif
> +
> +      nsAutoString pathStr;
> +      mozilla::Unused
> +        << NS_WARN_IF(NS_FAILED(NS_CopyNativeToUnicode(

'<<' on preceding line.

@@ +219,3 @@
>      }
>  
> +  } // scope for reading /proc/<pid>/maps

The #if/#endif already marks the start and end of this code. I think we don't need a braced scope as well.

Comment 3

[Julian Seward [:jseward]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Comment on attachment 8856516 [details] [diff] [review]
> Tidy up shared-linux-libraries.cc
> 
> Review of attachment 8856516 [details] [diff] [review]:

All review comments fixed.

> > +// There are three different configuration cases:
> 
> I think it might be even better if we #define constants to give these three
> cases names. (Like I did with PROFILE_JAVA.) Perhaps USE_ITERATE,
> USE_ITERATE_AND_MAPS, USE_MAPS?

Yes, nice.  I used CONFIG_CASE_{1,2,3} which looks pretty tidy.  When
we come to remove MOZ_WIDGET_GONK from the profiler, we'll have only two
cases left, and this can be simplified again, since the decision will
then only be Android-or-Linux.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97064fe644ca
Tidy up shared-linux-libraries.cc.  r=n.nethercote.

Comment 5

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/97064fe644ca