[PATCH][RFC] llvm-cov HTML generation
Harlan Haskins via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 17 13:27:45 PDT 2016
I’ve attached an updated patch, and (separately) the binary files it references.
I still have no idea how to properly attach them.
— Harlan
> On Mar 17, 2016, at 11:20 AM, Harlan Haskins via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
>
>> On Mar 17, 2016, at 5:35 AM, Ying Yi <maggieyi666 at gmail.com <mailto:maggieyi666 at gmail.com>> wrote:
>>
>> Thanks for updating the patch.
>>
>> 1) >If you could help me isolate why that might happen, I’d appreciate it.
>>
>> The variable Filename in the function getOutputPathForFile is a Twine. Parts of the Twine are being ‘lost’ in the release build which causes the issue. From the comments in the Twine.h header, “A Twine is not intended for use directly and should not be stored ... Twines should only be used accepted as const references in arguments”.
>>
>> To solve the issue, you could change the code to:
>> auto Filename = (sys::path::filename(SourceFile) + "." + FileExt).str();
>
> Thanks! I’ve updated my patch to reflect that.
>
>> 2) The two new tests failed since the lineExecutionCountsHTML.covmapping and templateInstantiationsHTML.covmapping files are missing.
>
> This is an issue. How can I embed those binary files in a patch?
>
>> 3) Could you add a test for '-format=text' with '-output-dir=...'?
>
> Yep, I’ll just modify the existing lineExecutionCounts test to also run outputting to a directory and running the same FileCheck on that file
>
>> 4) By the way, the patch includes an unnecessary change to the llvm-cov.cpp file.
>
> Fixed.
>
>> Thanks,
>>
>> Maggie
>
> I’ll shoot an updated patch once the tests pass and I can figure out how to add this binary file…
>
> Thanks again,
> Harlan
>
>> On Mon, Mar 14, 2016 at 6:11 PM, Harlan Haskins <hhaskins at apple.com <mailto:hhaskins at apple.com>> wrote:
>> I haven’t been able to reproduce this issue. Not quite sure why you’re seeing it.
>>
>> If you could help me isolate why that might happen, I’d appreciate it.
>>
>> In the meantime, a small final patch to silence that switch warning and avoid some unnecessary copies.
>>
>>
>>
>> Thanks,
>> Harlan
>>
>>> On Mar 11, 2016, at 1:14 AM, Ying Yi <maggieyi666 at gmail.com <mailto:maggieyi666 at gmail.com>> wrote:
>>>
>>> These are the steps that I used to get the issue:
>>>
>>> 1) Build:
>>> $ cmake -G "Unix Makefiles" -DCMAKE_INSTALL_PREFIX=prefix=~/upstream/bin -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On ~/upstream/llvm
>>> $ make
>>>
>>> 2) Test:
>>> $ cat test.cpp
>>> int main(int argc, char ** argv) {
>>> int x = 1;
>>>
>>> for (int i = 0; i < 1000; ++i)
>>> x *= 2+1;
>>>
>>> return x;
>>> }
>>>
>>> $ ~/upstream/build/bin/clang -O0 -fprofile-instr-generate=test.profraw -fcoverage-mapping "test.cpp"
>>> $ ./a.out
>>> $ ~/upstream/build/bin/llvm-profdata merge test.profraw -o merged.profdata
>>> $ ~/upstream/build/bin/llvm-cov show -format=html -output-dir=./report a.out -instr-profile=merged.profdata
>>>
>>>
>>> Thanks,
>>>
>>>
>>>
>>> On Thu, Mar 10, 2016 at 10:41 PM, Harlan Haskins <hhaskins at apple.com <mailto:hhaskins at apple.com>> wrote:
>>>
>>>> On Mar 10, 2016, at 1:30 PM, Ying Yi <maggieyi666 at gmail.com <mailto:maggieyi666 at gmail.com>> wrote:
>>>>
>>>> Thanks for your reply and the updated patch.
>>>>
>>>> 1) When I use the release build of llvm-cov to generate HTML files, all the HTML files are named “html”. This issue also exists on UNIX.
>>>>
>>> I haven’t seen this happen. Can you provide some steps to reproduce? Thanks!
>>>> 2) The following warning is given when building llvm-cov using Visual Studio 2013.
>>>>
>>>> “sourcecoverageview.cpp (108): warning C4715: 'llvm::SourceCoverageView::create' : not all control paths return a value”
>>>>
>>> That warning is incorrect. It’s a switch statement over both cases of an enum — I want this because if we add another format, say, OFJSON or something, I want a compiler warning telling me the switch case isn’t exhaustive.
>>>
>>> Here’s an updated patch with some small sanity tests added:
>>>
>>>
>>>
>>> Best,
>>> Harlan
>>>
>>>>
>>>> On Wed, Mar 9, 2016 at 12:50 AM, Harlan Haskins <hhaskins at apple.com <mailto:hhaskins at apple.com>> wrote:
>>>> Updated patch with unistd removed, utf-8, and `console` replaced with `text`.
>>>>
>>>> Thanks!
>>>> Harlan
>>>>
>>>>
>>>>> On Mar 8, 2016, at 10:55 AM, Justin Bogner <mail at justinbogner.com <mailto:mail at justinbogner.com>> wrote:
>>>>>
>>>>> Harlan Haskins <hhaskins at apple.com <mailto:hhaskins at apple.com>> writes:
>>>>>> Thanks again! Responses inline.
>>>>>>> On Mar 8, 2016, at 5:08 AM, Ying Yi <maggieyi666 at gmail.com <mailto:maggieyi666 at gmail.com>> wrote:
>>>>>>>
>>>>>>> Thanks for adding the index page. I have some comments:
>>>>>>>
>>>>>>> 1) The ‘unistd.h’ header won’t build on windows.
>>>>>>>
>>>>>> Thanks for catching that — I was using it to debug, but don’t need
>>>>>> it. I also don’t have a Windows machine to debug, so thank you for
>>>>>> investigating portability!
>>>>>>
>>>>>>> 2) The help text for ‘-format’ is different from llvm-profdata.exe
>>>>>>> tool, which uses the following style:
>>>>>>> -format=<html|console> - Format to output comparison ….
>>>>>>> Is there a standard way to do this?
>>>>>>>
>>>>>> LLVM’s CommandLine library renders the help text. Also, I noticed I
>>>>>> used ‘comparison’ instead of ‘coverage’. Thanks!
>>>>>>
>>>>>>> 3) Should ‘console’ be changed to ‘text’, because it doesn’t show in
>>>>>>> the console when you give an output directory?
>>>>>>>
>>>>>> Yeah, I think it could. I’ve cc’d Justin for his input — he originally
>>>>>> suggested Console.
>>>>>
>>>>> My only problem with "text" is that this includes ANSI escape sequences
>>>>> when it uses colour, so text seems a bit misleading. That said, it
>>>>> doesn't matter much - I'm fine with either.
>>>>>
>>>>>>> 4) Could you please use an html charset that supports UTF-8?
>>>>>>>
>>>>>> Yep. Thanks for catching that!
>>>>>>
>>>>>>> 5) The HTML report can look quite different to the source file if it
>>>>>>> includes tabs. Could you specify a smaller tab-size in the CSS file?
>>>>>>>
>>>>>> That’s a good question, and I don’t quite know the correct answer to
>>>>>> it. GitHub has a complicated set of rules for tab sizing that makes a
>>>>>> ‘best guess’ based on the source file type. I wonder if it’s a good
>>>>>> candidate for a command line switch?
>>>>>>
>>>>>>> In the future (not for this patch):
>>>>>>>
>>>>>>> 6) Could we support a user CSS file?
>>>>>>> Currently, the CSS rules are embedded into the html page. Could
>>>>>>> llvm-cov provide an option to use an external CSS file? This would
>>>>>>> give the user more flexibility.
>>>>>>>
>>>>>> I agree that it’d give the user more flexibility, but it’d be
>>>>>> difficult to do given the nature of the directory structure. Maybe, if
>>>>>> you know you’re gonna be running it on a server where the ‘root’ is
>>>>>> well-defined, we could just dump the css file to disk and put a
>>>>>> relative <link> tag in. But if it’s gonna be ten directories down,
>>>>>> we’ll need to have some logic in that replaces all the parent paths in
>>>>>> the link with /../..’s to make the file resolve. I think it can be
>>>>>> done, just with more input from the user about their intention with
>>>>>> hosting.
>>>>>>
>>>>>>> 7) Could we put the functions/lines summary information into the
>>>>>>> HTML files? As a rough example:
>>>>>>>
>>>>>>> Lines: 9/10 (90%)
>>>>>>> Functions: 1/1 (100%)
>>>>>>>
>>>>>> I think this totally can be done — it’s out of scope for this patch
>>>>>> (it’s already a pretty big patch as it is.)
>>>>>> Some kind of merging of ‘show’ and ‘report’ would be ideal. Maybe the
>>>>>> index page has at-a-glance information about each file?
>>>>>
>>>>> Yep, this sounds like a great idea, but it's probably more suited to a
>>>>> follow up patch.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Ying Yi
>>>> SN Systems Ltd - Sony Computer Entertainment Group.
>>>
>>>
>>>
>>>
>>>
>>> --
>>> Ying Yi
>>> SN Systems Ltd - Sony Computer Entertainment Group.
>>
>>
>>
>>
>>
>> --
>> Ying Yi
>> SN Systems Ltd - Sony Computer Entertainment Group.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160317/0dfa606f/attachment-0003.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-cov-html.diff
Type: application/octet-stream
Size: 49327 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160317/0dfa606f/attachment-0001.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160317/0dfa606f/attachment-0004.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-cov-html-inputs.zip
Type: application/zip
Size: 1335 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160317/0dfa606f/attachment-0001.zip>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160317/0dfa606f/attachment-0005.html>
More information about the llvm-commits
mailing list