Page MenuHomePhabricator

Add -fprofile-dir= to clang.
ClosedPublic

Authored by nlewycky on Aug 19 2016, 6:17 PM.

Details

Reviewers
compnerd
Summary

-fprofile-dir=path allows the user to specify where .gcda files should be emitted when the program is run. In particular, this is the first flag that causes the .gcno and .o files to have different paths, LLVM is extended to support this. -fprofile-dir= does not change the file name in the .gcno (and thus where lcov looks for the source) but it does change the name in the .gcda (and thus where the runtime library writes the .gcda file). It's different from a GCOV_PREFIX because a user can observe that the GCOV_PREFIX_STRIP will strip paths off of -fprofile-dir= but not off of a supplied GCOV_PREFIX.

To implement this we split -coverage-file into -coverage-data-file and -coverage-notes-file to specify the two different names. The !llvm.gcov metadata node grows from a 2-element form {string coverage-file, node dbg.cu} to 3-elements, {string coverage-notes-file, string coverage-data-file, node dbg.cu}. In the 3-element form, the file name is already "mangled" with .gcno/.gcda suffixes, while the 2-element form left that to the middle end pass.

Diff Detail

Event Timeline

nlewycky updated this revision to Diff 68753.Aug 19 2016, 6:17 PM
nlewycky updated this revision to Diff 68755.
nlewycky retitled this revision from to Add -fprofile-dir= to clang..
nlewycky updated this object.
nlewycky added subscribers: llvm-commits, cfe-commits.

Forgot clang changes!

Ping!

I think the review this patch really needs is a comparison to GCC's implementation to double-check that it really does the same thing that GCC does. Is there a gcov user who could take a look at this?

compnerd added inline comments.
lib/Transforms/Instrumentation/GCOVProfiling.cpp
121–122

Might be nice to use enum class.

422

I think that NewStem is no longer appropriate.

447

It really feels like these two cases can be collapsed.

tools/clang/lib/CodeGen/CodeGenModule.cpp
4221 ↗(On Diff #68755)

auto *CoverageDataFile perhaps?

4223 ↗(On Diff #68755)

Similar.

tools/clang/lib/Driver/Tools.cpp
3621 ↗(On Diff #68755)

Unnecessary braces.

nlewycky updated this revision to Diff 69778.Aug 30 2016, 4:18 PM
nlewycky marked 5 inline comments as done.
nlewycky added inline comments.
lib/Transforms/Instrumentation/GCOVProfiling.cpp
447

I don't see a great way to do that, so I picked the one which minimizes the cost of the common case. Having an !llvm.gcov node but no CU for the specific .o is uncommon: to get here you're building a program with some .bc's built with a frontend that emits an !llvm.gcov and some .bc's without, and then you llvm-linked them and are running the insert-gcov-profiling pass afterwards. Also, I assume that copying SmallString<128>'s around is slow, while looking up Value*'s is faster.

If you have a specific idea, I'm happy to try it. I'm not thrilled with the way it's written now.

test/Transforms/GCOVProfiling/three-element-mdnode.ll
28

The way this test was written, !10 would be parsed before !9. Swizzled it around to make !9 the !llvm.module.flags and !10 the !llvm.gcov node.

compnerd added inline comments.Aug 30 2016, 9:12 PM
lib/Transforms/Instrumentation/GCOVProfiling.cpp
447

A quick pass over this yielded something like:

if (N->getNumOperands() != 2 && N->getNumOperands() != 3)
  continue;

bool HasProfileDir = N->getNumOperands() == 3;
unsigned Offset = HasProfileDir ? 1 : 0;

auto *NotesFile = dyn_cast<MDString>(N->getOperand(Offset + 0));
auto *CovFile = dyn_cast<MDString>(N->getOperand(Offset + 0));
auto *CompileUnit = dyn_cast<MDNode>(N->getOperand(Offset + 1));

if ((!HasProfileDir || !NotesFile) || !CovFile || !CompileUnit)
  continue;

if (CompileUnit == CU) {
  bool GCNO = OutputType == GCovFileType::GCNO;
  if (HasProfileDir) {
    return GCNO ? NotesFile->getString() : CovFile->getString();
  } else {
    SmallString<128> FileName = CovFile->getString();
    sys::path::replace_extension(FileName, GCNO ? "gcno" : "gcda");
    return FileName.str();
  }
}

It does reduce a bit of the duplication, and still accounts for the string duplication that you were pointing out. I think it makes it slightly easier to read due to the variable de-duplication (at the slight cost of an extra pointer on the stack and an extra lookup).

nlewycky updated this revision to Diff 69905.Aug 31 2016, 2:36 PM
compnerd accepted this revision.Aug 31 2016, 3:47 PM
compnerd added a reviewer: compnerd.
This revision is now accepted and ready to land.Aug 31 2016, 3:47 PM
Eugene.Zelenko closed this revision.Oct 4 2016, 3:55 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL280306. Please specify "Differential revision: <URL>" as last line of commit message.