This is an archive of the discontinued LLVM Phabricator instance.

sancov -not-covered-functions.
ClosedPublic

Authored by aizatsky on Nov 20 2015, 2:27 PM.

Details

Summary

The command prints out list of functions that were not entered.

Diff Detail

Event Timeline

aizatsky updated this revision to Diff 40830.Nov 20 2015, 2:27 PM
aizatsky retitled this revision from to sancov -not_covered_functions..
aizatsky updated this object.
aizatsky added a subscriber: llvm-commits.
krasin edited edge metadata.Nov 23 2015, 9:38 AM

First bunch of comments from Friday. Will add more later.

tools/sancov/sancov.cc
64

LLVM tools use flags (with a notable exception of llvm-dwarfdump) with a dash instead of an underscore.
Please rename the flag to --not-covered-functions, and also do the same with --covered-functions.

114–120

Ditto, exit(1)

114–120

Why not exit(1)? LLVM tools use 1 as the exit code in case of an error:

ag "exit[(]1[)]" tools | wc -l
109

There's 3 python scripts which violate that, but otherwise the rule is followed pretty much everywhere.

427–438

Please fix the comment: "Print list of not covered functions".

429

Per LLVM style guide: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).

Please, change everywhere (except main)

aizatsky updated this revision to Diff 41000.Nov 23 2015, 5:13 PM
aizatsky marked 5 inline comments as done.

review

krasin added inline comments.Nov 23 2015, 5:13 PM
tools/sancov/sancov.cc
566

What if some file names are in /usr/include? This could happen if some of the functions are inlined from a system header.

All done. PTAL

aizatsky retitled this revision from sancov -not_covered_functions. to sancov -not-covered-functions..Nov 23 2015, 5:15 PM
krasin added inline comments.Nov 23 2015, 5:37 PM
tools/sancov/sancov.cc
379

FailIfError, as implemented right now, drops the context in which it occurred. I would expect something like:

Error: The end of the file was unexpectedly encountered (4)

Please, add an additional string param (like Message) and print it into stderr. In this case the invocation will be like

FailIfError("symbolizeInlinedCode: " + ClBinaryName, InliningInfo);

and the error message will suddenly become much more useful:

Error: symbolizeInlinedCode: lala.o: The end of the file was unexpectedly encountered (4)

Note: feel free to improve the actual error message format. The point is to have all the needed data points in the error message. Otherwise, we'll waste our own time later.

pcc added inline comments.Nov 23 2015, 5:52 PM
lib/Object/ObjectFile.cpp
118 ↗(On Diff #41002)

I'd probably prefer if this function returned a Target * to reflect the fact that it isn't really supposed to (nor can it) return an accurate triple but rather something that can be used to summon appropriate bits of MC etc.

krasin added inline comments.Nov 23 2015, 6:23 PM
tools/sancov/sancov.cc
379

You may also consider to have a macro instead of a function, as it would allow to preserve the code of line. See for example: https://github.com/llvm-mirror/llvm/blob/358918e8dd375e41b4f51e7570a57e123b2951c6/unittests/Support/Path.cpp#L31

The macro in the example is gtest-specific, but it still gives an idea.

449

Why is lookupTarget error critical, while an error in createMCSubtargetInfo is not?

471

Please, remove this empty line. There's no empty line in the similar places above.

484

Could createMCInstrAnalysis fail? If yes, please, add error handling as you did above.

489

A comment why do we skip virtual .text sections is appreated.

497

The more I think, the more it looks to me that an assert or CHECK should be used here. The problem is that asserts are disabled in some environments, and it would be unwise to proceed here.

517

Please, add a better TODO description (if you wish to address some issue here later)

518

A comment about the way you compute this address is appreciated.

569

What happens if you only have a single file? I have a feeling that the file name will be printed as an empty string.

samsonov edited edge metadata.Nov 24 2015, 10:37 AM

Mostly fine, several nits below (in addition to Ivan's comments).

I don't quite agree on the change to LLVMObject, that method looks somewhat weird to me. We can probably discuss that separately: for now you seem to only support x86_64-linux anyway.

lib/Object/ObjectFile.cpp
118 ↗(On Diff #41002)

Agree: you can't rely deduce triple from object file. At the very least, you should reflect that you're guessing the triple in method name.

tools/sancov/sancov.cc
121

nit: please use "static" keyword consistently: it's not really needed for functions, as they are enclosed in anonymous namespace, but if you're willing to keep it - keep it everywhere.

358

Nit: LLVM tends to use RHS variant (as it's an abbreviation).

$ grep -rnH "RHS" lib/* | wc -l
10263
$ grep -rnH "Rhs" lib/* | wc -l
4

396

return llvm::make_unique<CoverageData>(std::move(Addrs));

424

Just *Addrs

432

llvm_unreachable()

480

Add llvm_unreachable() at the end?

574

TBH I don't understand the rationale behind stripping (calculated) common prefix. I can construct weird examples, e.g. imagine I have built my executable from /tmp/a.cc with function foo() and /tmp2/a.cc with function foo(). Suppose I suspect that when I ran my executable only one was executed. I call llvm-cov my.exe and see

a.cc:1 foo()

Well, thanks :)

580

Just FYI: file names may contain colons, as well as spaces. It's probably not a big deal, but just note that the output of llvm-cov might be not machine-readable.

pcc added inline comments.Nov 24 2015, 10:41 AM
lib/Object/ObjectFile.cpp
118 ↗(On Diff #41002)

Actually it seems that since the MC constructors use the target triple string we should return a triple string here at least for now.

aizatsky updated this revision to Diff 41065.Nov 24 2015, 10:59 AM
aizatsky marked 4 inline comments as done.
aizatsky edited edge metadata.

review

ptal

lib/Object/ObjectFile.cpp
118 ↗(On Diff #41002)

Sorry, can't do. Triple name is used to obtain various pieces of MC.
Plus target lookup changes Triple object (!). That's why I can't simply return std::string instead of Triple here. I've added the clarifying comment to the .h files about the function.

tools/sancov/sancov.cc
379

This doesn't seem to be the way things are done in llvm tools. I'd keep it as is if you don't mind.

489

I have no idea. This is how it is in objdump. Do you know why?

569

It won't see the if (FileName.size() > FilePrefix.size()) below.

aizatsky marked 3 inline comments as done.Nov 24 2015, 11:05 AM

ptal

lib/Object/ObjectFile.cpp
118 ↗(On Diff #41002)

I don't feel attached to it. If you'd rather see me copy this code - I'm fine.

tools/sancov/sancov.cc
396

can't do because CoverageData constructor is private.

480

again, why?

samsonov added inline comments.Nov 24 2015, 11:20 AM
lib/Object/ObjectFile.cpp
118 ↗(On Diff #41067)

Yeah, let's copy the code, and remove the cases we don't really support yet (MachO/COFF).

tools/sancov/sancov.cc
480

See http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations
Also, it serves as an annotation that you're not supposed to exit from switch.

aizatsky marked 2 inline comments as done.Nov 24 2015, 11:28 AM

ptal

LGTM, passing this back to Ivan.

krasin added inline comments.Nov 24 2015, 12:10 PM
tools/sancov/sancov.cc
350

I also think that it's not the right feature. If we want strip prefixes, that should be a flag that specifies the prefix, not auto-magic.

379

Can you please give me examples of that in llvm tools?

489

No I don't. Please, at least, add a comment that the code was stolen from objdump. Please, give a more precise location, so that it would be possible to lookup later for a reader that is not you or me.

aizatsky updated this revision to Diff 41075.Nov 24 2015, 12:34 PM

extracted path::common_prefix.

I have extracted path::common_prefix for all of you nitpickers :P

aizatsky updated this revision to Diff 41574.Dec 1 2015, 3:52 PM

removed common path functionality.

I removed common path functionality and implemented strip_path_prefix flag instead. PTAL.

krasin added a subscriber: krasin.Dec 1 2015, 3:56 PM

Please, mark the comments which are addressed as done. It will make it
easier to follow the review.

This revision is now accepted and ready to land.Dec 14 2015, 2:12 PM
aizatsky closed this revision.Dec 14 2015, 2:12 PM