[lld] r307459 - Delete the pdb diff test.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 10 10:51:11 PDT 2017


On Mon, Jul 10, 2017 at 10:38 AM Zachary Turner <zturner at google.com> wrote:

> 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?
>

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.

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).


>  :)  In any case, I should have a fix in the next couple hours.
>
> On Mon, Jul 10, 2017 at 10:21 AM David Blaikie <dblaikie at gmail.com> wrote:
>
>> 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)
>>
>> On Mon, Jul 10, 2017 at 10:20 AM Zachary Turner <zturner at google.com>
>> wrote:
>>
>>> The functionality actually works, it's just a problem with the way the
>>> test is written which is not entirely easy to fix.
>>>
>>> On Mon, Jul 10, 2017 at 10:18 AM David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> 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.
>>>>
>>>> On Fri, Jul 7, 2017 at 7:54 PM Zachary Turner via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: zturner
>>>>> Date: Fri Jul  7 19:54:19 2017
>>>>> New Revision: 307459
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=307459&view=rev
>>>>> Log:
>>>>> Delete the pdb diff test.
>>>>>
>>>>> This is failing on Linux for unknown reasons, so I need to
>>>>> get the bots green while I investigate.
>>>>>
>>>>> Removed:
>>>>>     lld/trunk/test/COFF/pdb-diff.test
>>>>>
>>>>> Removed: lld/trunk/test/COFF/pdb-diff.test
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/lld/trunk/test/COFF/pdb-diff.test?rev=307458&view=auto
>>>>>
>>>>> ==============================================================================
>>>>> --- lld/trunk/test/COFF/pdb-diff.test (original)
>>>>> +++ lld/trunk/test/COFF/pdb-diff.test (removed)
>>>>> @@ -1,212 +0,0 @@
>>>>> -This test verifies that we produce PDBs compatible with MSVC in
>>>>> various ways.
>>>>> -We check in a cl-generated object file, PDB, and original source
>>>>> which serve
>>>>> -as the "baseline" for us to measure against.  Then we link the same
>>>>> object
>>>>> -file with LLD and compare the two PDBs.  Since the baseline object
>>>>> file and
>>>>> -PDB are already checked in, we just run LLD on the object file.
>>>>> -
>>>>> -RUN: lld-link /debug /pdb:%T/pdb-diff-lld.pdb /nodefaultlib
>>>>> /entry:main %S/Inputs/pdb-diff.obj
>>>>> -RUN: llvm-pdbutil diff -result -values=false %T/pdb-diff-lld.pdb
>>>>> %S/Inputs/pdb-diff-cl.pdb | FileCheck %s
>>>>> -
>>>>> -CHECK:        ----------------------
>>>>> -CHECK-NEXT:   |  MSF Super Block   |
>>>>> -CHECK-NEXT:   |----------------+---|
>>>>> -CHECK-NEXT:   |           File |   |
>>>>> -CHECK-NEXT:   |----------------+---|
>>>>> -CHECK-NEXT:   |     Block Size | I |
>>>>> -CHECK-NEXT:   |----------------+---|
>>>>> -CHECK-NEXT:   |    Block Count |
>>>>> -CHECK-NEXT:   |----------------+---|
>>>>> -CHECK-NEXT:   |      Unknown 1 | I |
>>>>> -CHECK-NEXT:   |----------------+---|
>>>>> -CHECK-NEXT:   | Directory Size |
>>>>> -CHECK-NEXT:   |----------------+---|
>>>>> -CHECK-NEXT:   ------------------------------------
>>>>> -CHECK-NEXT:   |         Stream Directory         |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                         File |   |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                 Stream Count | D |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |            Old MSF Directory | I |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                   PDB Stream | I |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                   TPI Stream | I |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                   DBI Stream | I |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                   IPI Stream | I |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                 New FPO Data | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |          Section Header Data | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |        Named Stream "/names" | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |     Named Stream "/LinkInfo" | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   | Module "{{.*}}\pdb-diff.obj" | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |          Module "* Linker *" | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                     TPI Hash | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                     IPI Hash | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |           Global Symbol Hash | D |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |           Public Symbol Hash | D |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |        Public Symbol Records | D |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   ------------------------------------
>>>>> -CHECK-NEXT:   |           String Table           |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                         File |   |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |            Number of Strings | D |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                 Hash Version | I |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                    Byte Size |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                    Signature | I |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |                Empty Strings |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |  {{.*}}pdb-diff.cpp | {{[EI]}} |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |  $T0 $ebp = $...p $T0 8 + =  | D |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   |  d:\src\llvm-...er internal) | D |
>>>>> -CHECK-NEXT:   |------------------------------+---|
>>>>> -CHECK-NEXT:   ----------------------------
>>>>> -CHECK-NEXT:   |        PDB Stream        |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |                 File |   |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |          Stream Size |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |                  Age | I |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |                 Guid | D |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |            Signature | D |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |              Version | I |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |       Features (set) | I |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |              Feature | I |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |    Named Stream Size |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |  Named Streams (map) | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |               /names | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   |            /LinkInfo | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------+---|
>>>>> -CHECK-NEXT:   ----------------------------------------------
>>>>> -CHECK-NEXT:   |                 DBI Stream                 |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                                   File |   |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                            Dbi Version | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                                    Age | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                                Machine | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                                  Flags | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                            Build Major | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                            Build Minor | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                           Build Number | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        PDB DLL Version | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                           PDB DLL RBLD | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                              DBG (FPO) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        DBG (Exception) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                            DBG (Fixup) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        DBG (OmapToSrc) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                      DBG (OmapFromSrc) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                       DBG (SectionHdr) | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                      DBG (TokenRidMap) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                            DBG (Xdata) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                            DBG (Pdata) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                           DBG (NewFPO) | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                   DBG (SectionHdrOrig) | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                         Globals Stream | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                         Publics Stream | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                         Symbol Records | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                             Has CTypes | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                Is Incrementally Linked | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                            Is Stripped | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                           Module Count | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                      Source File Count | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |Module "{{.*}}\pdb-diff.obj"|
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                                 - Modi | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        - Obj File Name | {{[DEI]}} |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                         - Debug Stream | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        - C11 Byte Size | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        - C13 Byte Size | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                           - # of files | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                  - Pdb File Path Index | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |               - Source File Name Index | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                     - Symbol Byte Size | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |            Module "* Linker *"             |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                                 - Modi | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        - Obj File Name | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                         - Debug Stream | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        - C11 Byte Size | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                        - C13 Byte Size | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                           - # of files | I |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                  - Pdb File Path Index | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |               - Source File Name Index | {{[EI]}} |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -CHECK-NEXT:   |                     - Symbol Byte Size | D |
>>>>> -CHECK-NEXT:   |----------------------------------------+---|
>>>>> -
>>>>> -
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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/20170710/5a3c4511/attachment.html>


More information about the llvm-commits mailing list