[WIP] fix xdebug coverage (part2): support XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE #7900

xKerman · 2017-07-02T09:13:57Z

description

related to: #1589 and #7888

This pull request adds support for XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE flags to xdebug_start_code_coverage() .

example

With this pull request, code coverage report for moneyphp/money can be generated with HHVM + PHPUnit as following:
https://xkerman.github.io/hhvm-coverage-sample/hhvm/

Contrast coverage report genereated with PHP5.6 + PHPUnit:
https://xkerman.github.io/hhvm-coverage-sample/php56/

Note: In order to generate coverage report with HHVM + PHPUnit, I need to apply the following patch to phpunit/php-code-coverage:

diff --git a/src/Driver/HHVM.php b/src/Driver/HHVM.php
index b35ea81..85dc6a9 100644
--- a/src/Driver/HHVM.php
+++ b/src/Driver/HHVM.php
@@ -24,6 +24,6 @@ class HHVM extends Xdebug
      */
     public function start($determineUnusedAndDead = true)
     {
-        xdebug_start_code_coverage();
+        xdebug_start_code_coverage(XDEBUG_CC_UNUSED | XDEBUG_CC_DEAD_CODE);
     }

xKerman · 2017-07-16T21:45:35Z

2 weeks have passed since I submitted this pull request.
@mofarrell could you review this?

hhvm-bot · 2017-07-24T15:46:21Z

@mofarrell has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

xKerman · 2017-07-24T16:16:53Z

Thanks, but I found there is a conflict. I will rebase this pull request.

hhvm-bot · 2017-07-24T20:18:16Z

@xKerman updated the pull request - view changes - changes since last import

facebook-github-bot · 2017-07-28T16:04:18Z

@mofarrell has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

xKerman · 2017-08-14T14:02:49Z

@mofarrell It has been over 2 weeks since this pull request was imported to Phabricator.
I would appreciate it if you let me know whether any updates are needed.

mofarrell · 2017-08-14T21:21:09Z

hphp/runtime/base/code-coverage.cpp

@@ -129,6 +160,42 @@ Array CodeCoverage::Report(bool sys /* = true */) {
  return ret;
 }

+Array CodeCoverage::ReportExecutable() {


Both this and ReportDeadCode could go since all the bookkeeping would happen in m_hits, and be reported by Report.

mofarrell · 2017-08-14T21:22:51Z

hphp/runtime/vm/bytecode.cpp

+  auto lines = unit->getLinesWithCode();
+  TI().m_coverage->RecordAsNotDeadCode(filename, std::move(lines));
+}
+


These could go away, and their logic could be performed when initializing a new std::vector for the file in m_hits.

mofarrell · 2017-08-14T21:24:29Z

hphp/runtime/vm/bytecode.cpp

+      recordNotDeadCode(unit, filename);
+    }
+  }
+


This could be removed. The filter for initializing a new m_hits entry would handle this.
Also this will recompute the DeadCode, and ExecutableLines every time we switch back to a unit.

mofarrell · 2017-08-14T21:29:29Z

hphp/runtime/ext/xdebug/ext_xdebug.cpp

+  for (ArrayIter iter(merged); iter; ++iter) {
+    Array tmp = iter.second().toArray();
+    tmp->ksort(SORT_REGULAR, true);
+    ret.set(iter.first(), tmp);


This should all be able to be left the same. Some filtering may need to happen for the DEAD or UNUSED values if the recording was being performed with it, then stopped without throwing away data, then restarted without DEAD and UNUSED data. I don't quite know xdebug's exact behavior there, but it might not show the older DEAD or UNUSED values.

mofarrell · 2017-08-14T21:34:12Z

hphp/runtime/base/request-injection-data.h

@@ -288,6 +300,8 @@ struct RequestInjectionData {

  bool m_debuggerAttached{false};
  bool m_coverage{false};
+  bool m_coverageWithUnused{false};


Enum with possible states for coverage recording.

mofarrell · 2017-08-14T21:35:33Z

hphp/runtime/base/request-injection-data-inl.h

+  m_coverageWithDeadCode = flag;
+  updateJit();
+}
+


All unneeded if collapsed with enum.

mofarrell · 2017-08-14T21:38:01Z

@xKerman Sorry this has taken so long. I kept checking back after test results had expired. This looks like it is working well, but I feel like it would be cleaner to do all the book keeping in a single map. Rather than merging them at the end.

xKerman · 2017-08-14T23:15:35Z

@mofarrell Thanks for your code review and comments.

it would be cleaner to do all the book keeping in a single map. Rather than merging them at the end.

I see, I will try to change to use a single map (m_hits) for reporting code coverage.

hhvm-bot added CLA Signed GH Review: review-needed labels Jul 2, 2017

hhvm-bot added the Import Started label Jul 24, 2017

xKerman added 2 commits July 25, 2017 01:05

add support for XDEBUG_CC_UNUSED

5795b07

add support for XDEBUG_CC_DEAD_CODE

318a34c

xKerman force-pushed the feature/xdebug-unused-and-deadcode branch from 800f49a to 318a34c Compare July 24, 2017 20:18

mofarrell requested changes Aug 14, 2017

View reviewed changes

xKerman changed the title ~~fix xdebug coverage (part2): support XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE~~ [WIP] fix xdebug coverage (part2): support XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE Aug 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] fix xdebug coverage (part2): support XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE #7900

[WIP] fix xdebug coverage (part2): support XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE #7900

xKerman commented Jul 2, 2017 •

edited

Loading

xKerman commented Jul 16, 2017

hhvm-bot commented Jul 24, 2017

xKerman commented Jul 24, 2017

hhvm-bot commented Jul 24, 2017

facebook-github-bot commented Jul 28, 2017

xKerman commented Aug 14, 2017

mofarrell Aug 14, 2017

mofarrell Aug 14, 2017

mofarrell Aug 14, 2017

mofarrell Aug 14, 2017

mofarrell Aug 14, 2017

mofarrell Aug 14, 2017

mofarrell commented Aug 14, 2017

xKerman commented Aug 14, 2017

[WIP] fix xdebug coverage (part2): support XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE #7900

Are you sure you want to change the base?

[WIP] fix xdebug coverage (part2): support XDEBUG_CC_UNUSED and XDEBUG_CC_DEAD_CODE #7900

Conversation

xKerman commented Jul 2, 2017 • edited Loading

description

example

xKerman commented Jul 16, 2017

hhvm-bot commented Jul 24, 2017

xKerman commented Jul 24, 2017

hhvm-bot commented Jul 24, 2017

facebook-github-bot commented Jul 28, 2017

xKerman commented Aug 14, 2017

mofarrell Aug 14, 2017

Choose a reason for hiding this comment

mofarrell Aug 14, 2017

Choose a reason for hiding this comment

mofarrell Aug 14, 2017

Choose a reason for hiding this comment

mofarrell Aug 14, 2017

Choose a reason for hiding this comment

mofarrell Aug 14, 2017

Choose a reason for hiding this comment

mofarrell Aug 14, 2017

Choose a reason for hiding this comment

mofarrell commented Aug 14, 2017

xKerman commented Aug 14, 2017

xKerman commented Jul 2, 2017 •

edited

Loading