<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 10, 2017 at 10:38 AM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">The test isn't testing the functionality that was committed. The functionality that was committed was committed in order to test existing functionality that has never had test coverage to begin with. So I think it comes down to "where do you draw the line". I could commit tests for the functionality that is there specifically to test other functionality, and would I then need to commit tests for that too?</div></blockquote><div><br>Sure enough - dwarfdump doesn't have direct testing for all its functionality, sometimes functionality's added and used to test the production functionality without ever testing that dwarfdump behaves appropriately on example/known input, etc. I'm not hugely hardline about that - but there is /some/ direct testing - probably good to have more.<br><br>In any case, if what you're describing is the case here - reverting the whole patch doesn't seem problematic - the functionality that was committed is unused if the test is removed (since it's only there to facilitate testing). Seems easier to keep track of things if it's reverted/recommitted wholesale (easier to find related code/changes when looking through log history, etc).<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"> :) In any case, I should have a fix in the next couple hours.</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 10, 2017 at 10:21 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Sure - but functionality without test coverage isn't OK (in the same way the patch wouldn't be acceptable if it didn't have a test to start with, committing it then removing the test coverage doesn't seem OK to me)</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 10, 2017 at 10:20 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">The functionality actually works, it's just a problem with the way the test is written which is not entirely easy to fix.</div><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 10, 2017 at 10:18 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Generally I would expect a whole patch should probably be reverted if the tests are failing - rather than undoing/removing the tests but leaving the functionality in-tree.</div><br><div class="gmail_quote"><div dir="ltr">On Fri, Jul 7, 2017 at 7:54 PM Zachary Turner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: zturner<br>
Date: Fri Jul 7 19:54:19 2017<br>
New Revision: 307459<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=307459&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=307459&view=rev</a><br>
Log:<br>
Delete the pdb diff test.<br>
<br>
This is failing on Linux for unknown reasons, so I need to<br>
get the bots green while I investigate.<br>
<br>
Removed:<br>
lld/trunk/test/COFF/pdb-diff.test<br>
<br>
Removed: lld/trunk/test/COFF/pdb-diff.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/test/COFF/pdb-diff.test?rev=307458&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/test/COFF/pdb-diff.test?rev=307458&view=auto</a><br>
==============================================================================<br>
--- lld/trunk/test/COFF/pdb-diff.test (original)<br>
+++ lld/trunk/test/COFF/pdb-diff.test (removed)<br>
@@ -1,212 +0,0 @@<br>
-This test verifies that we produce PDBs compatible with MSVC in various ways.<br>
-We check in a cl-generated object file, PDB, and original source which serve<br>
-as the "baseline" for us to measure against. Then we link the same object<br>
-file with LLD and compare the two PDBs. Since the baseline object file and<br>
-PDB are already checked in, we just run LLD on the object file.<br>
-<br>
-RUN: lld-link /debug /pdb:%T/pdb-diff-lld.pdb /nodefaultlib /entry:main %S/Inputs/pdb-diff.obj<br>
-RUN: llvm-pdbutil diff -result -values=false %T/pdb-diff-lld.pdb %S/Inputs/pdb-diff-cl.pdb | FileCheck %s<br>
-<br>
-CHECK: ----------------------<br>
-CHECK-NEXT: | MSF Super Block |<br>
-CHECK-NEXT: |----------------+---|<br>
-CHECK-NEXT: | File | |<br>
-CHECK-NEXT: |----------------+---|<br>
-CHECK-NEXT: | Block Size | I |<br>
-CHECK-NEXT: |----------------+---|<br>
-CHECK-NEXT: | Block Count |<br>
-CHECK-NEXT: |----------------+---|<br>
-CHECK-NEXT: | Unknown 1 | I |<br>
-CHECK-NEXT: |----------------+---|<br>
-CHECK-NEXT: | Directory Size |<br>
-CHECK-NEXT: |----------------+---|<br>
-CHECK-NEXT: ------------------------------------<br>
-CHECK-NEXT: | Stream Directory |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | File | |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Stream Count | D |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Old MSF Directory | I |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | PDB Stream | I |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | TPI Stream | I |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | DBI Stream | I |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | IPI Stream | I |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | New FPO Data | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Section Header Data | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Named Stream "/names" | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Named Stream "/LinkInfo" | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Module "{{.*}}\pdb-diff.obj" | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Module "* Linker *" | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | TPI Hash | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | IPI Hash | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Global Symbol Hash | D |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Public Symbol Hash | D |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Public Symbol Records | D |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: ------------------------------------<br>
-CHECK-NEXT: | String Table |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | File | |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Number of Strings | D |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Hash Version | I |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Byte Size |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Signature | I |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | Empty Strings |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | {{.*}}pdb-diff.cpp | {{[EI]}} |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | $T0 $ebp = $...p $T0 8 + = | D |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: | d:\src\llvm-...er internal) | D |<br>
-CHECK-NEXT: |------------------------------+---|<br>
-CHECK-NEXT: ----------------------------<br>
-CHECK-NEXT: | PDB Stream |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | File | |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Stream Size |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Age | I |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Guid | D |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Signature | D |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Version | I |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Features (set) | I |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Feature | I |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Named Stream Size |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | Named Streams (map) | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | /names | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: | /LinkInfo | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------+---|<br>
-CHECK-NEXT: ----------------------------------------------<br>
-CHECK-NEXT: | DBI Stream |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | File | |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Dbi Version | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Age | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Machine | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Flags | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Build Major | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Build Minor | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Build Number | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | PDB DLL Version | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | PDB DLL RBLD | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (FPO) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (Exception) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (Fixup) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (OmapToSrc) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (OmapFromSrc) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (SectionHdr) | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (TokenRidMap) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (Xdata) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (Pdata) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (NewFPO) | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | DBG (SectionHdrOrig) | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Globals Stream | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Publics Stream | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Symbol Records | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Has CTypes | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Is Incrementally Linked | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Is Stripped | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Module Count | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Source File Count | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: |Module "{{.*}}\pdb-diff.obj"|<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Modi | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Obj File Name | {{[DEI]}} |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Debug Stream | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - C11 Byte Size | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - C13 Byte Size | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - # of files | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Pdb File Path Index | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Source File Name Index | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Symbol Byte Size | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | Module "* Linker *" |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Modi | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Obj File Name | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Debug Stream | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - C11 Byte Size | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - C13 Byte Size | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - # of files | I |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Pdb File Path Index | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Source File Name Index | {{[EI]}} |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-CHECK-NEXT: | - Symbol Byte Size | D |<br>
-CHECK-NEXT: |----------------------------------------+---|<br>
-<br>
-<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div>
</blockquote></div></div>