[llvm-commits] PATCH: add new test tool: obj2yaml

[Marshall Clow]

On Jun 12, 2012, at 1:40 PM, Michael Spencer wrote:

> On Tue, Jun 12, 2012 at 8:35 AM, Marshall Clow <mclow.lists at gmail.com> wrote:
>> On Jun 9, 2012, at 2:44 AM, Michael Spencer wrote:
>> 
>>> On Fri, Jun 8, 2012 at 11:09 AM, Marshall Clow <mclow.lists at gmail.com> wrote:
>>>> [ resending to llvm-commits instead of -dev ]
>>>> 
>>>> This patch adds a tool called 'obj2yaml', which takes an object file, and produces a YAML representation of the file.
>>>> This is a companion tool to Michael Spencer's proposed "yaml2obj" tool.
>>>> 
>>>> This tool processes only COFF files; the plan is to enhance it to handle other object formats, as well as object archives.
>>>> The primary uses for this tool is expected to be:
>>>>        1) Debugging object files that are produced by llvm and lld.
>>>>        2) Automated testing suites.
>>>> 
>>>> The goal is to have obj2yaml and yaml2obj be inverse operations, so that one can test either by using the other.
>>>> So, both obj -> yaml -> obj and yaml -> obj -> yaml are expected to be idempotent.
>>>> 
>>>> -- Marshall
>>>> 
>>>> Marshall Clow     Idio Software   <mailto:mclow.lists at gmail.com>
>>>> 
>>>> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
>>>>       -- Yu Suzuki
>>> 
>>> The design looks good, however there are quite a few LLVM style issues.
>> 
>> Extensively reworked. Now uses the facilities in llvm/Object/COFF.h for byte-sex awareness.
>> Still a few 80 column issues, and the relocation dumping is not done - but since yaml2obj doesn't build relocation entries yet, this may be moot. ;-)
>> 
>> -- Marshall
>> 
>> Marshall Clow     Idio Software   <mailto:mclow.lists at gmail.com>
>> 
>> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
>>        -- Yu Suzuki
> 
> The overall structure looks good now, but there are still quite a few style
> issues other than 80-col.

[ Major snippage ]

> 
> Index: utils/obj2yaml/obj2yaml.cpp
> 

> +#include <iostream>
> 
> iostream is only used in two places. Those should be replaced with llvm::errs()
> and this header removed.

Done.

> +//  Some useful stuff that's not in Support/COFF.h
> +namespace llvm { namespace COFF {
> 
> These should be on separate lines.
> 
> +enum {
> +  OptionalHeaderSize     = 28
> +};
> 
> This should be added to Support/COFF.h.

All removed.

> +void makeMaps () {
> +  assert(!MapsMade);
> +//  I'd rather that these were static_assert
> +  assert(sizeof(header) == HeaderSize);
> +  assert(sizeof(section) == SectionSize);
> +//  Using clang on MacOS, these are not true
> +//  assert(sizeof(symbol) == SymbolSize);
> +//  assert(sizeof(relocation) == RelocationSize);
> 
> These sizeof asserts can all be removed now, as they are unused.
> 
> +
> +  MachineTypeNames        = buildMap(MachineTypePairs);
> +  SectionCharacteristicNamesAlignment =
> buildMap(SectionCharacteristicsPairsAlignment);
> +  SymbolBaseTypeNames     = buildMap(SymbolBaseTypePairs);
> +  SymbolComplexTypeNames  = buildMap(SymbolComplexTypePairs);
> +  SymbolStorageClassNames = buildMap(SymbolStorageClassPairs);
> +  MapsMade = true;
> +}
> +
> +
> +}}
> 
> This namespace is long enough that these should be separate and have
> // end namespace x
> 

All gone.


> +  for (llvm::ArrayRef<uint8_t>::const_iterator iter = arr.begin ();
> +                    iter != arr.end (); ++iter)
> 
> arr.end should not be evaluated each time through the loop.

I disagree. Here's the implementation of ArrayRef.end ():

    iterator end() const { return Data + Length; }

That will be inlined, and since the ArrayRef is const, the pointer arithmetic will be hoisted out of the loop.


> +llvm::raw_ostream &writeName(llvm::raw_ostream &Out,
> +                             const char *Name, std::size_t NameSize) {
> 
> This isn't needed. You should just always use the StringRef version.
> 

> +  for (std::size_t i = 0; i < NameSize; ++i) {
> +    if (!Name[i]) break;
> +    Out << Name[i];
> +  }
> +  return Out;

The way that I read the spec, a COFF name is a series of characters, either null terminated, or "NameSize" bytes long.
This routine handles both cases.

> 
> +}
> +
> +llvm::raw_ostream &writeName(llvm::raw_ostream &Out, llvm::StringRef name) {
> +  for ( llvm::StringRef::const_iterator iter = name.begin ();
> +                      iter != name.end (); ++iter )
> +    Out << *iter;
> 
> This loop isn't needed. Out << name; works.
> 
> +  return Out;
> +}

Does llvm::raw_ostream know about StringRefs? Cool.
writeName has been removed.

> 
> +/*
> +    if (sect->NumberOfRelocations > 0) {
> +      Out << "    Relocations: " << endl;
> +      for (std::size_t i = 0; i < sect->NumberOfRelocations; ++i) {
> +        const coff_relocation *p ; // FIXME!
> +        Out << "      - !Relocation" << endl;
> +        Out << "        VirtualAddress: " ;
> +        yaml::writeHexNumber(p->VirtualAddress) << endl;
> +        Out << "        SymbolTableIndex: " << p->SymbolTableIndex << endl;
> +        Out << "        Type: " << endl;
> +        Out << endl;
> +      }
> +*/
> 
> Just remove the code instead of commenting it out. Leave a FIXME for adding
> relocation dumping.

I implemented it instead.
> 
> +//  TODO: If this is an archive, then burst it and dump each entry
> +  if (error_code ec = MemoryBuffer::getFileOrSTDIN(InputFilename, buf))
> +    std::cerr << "Error " << ec << " opening file '" << InputFilename
> << "' for reading" << endl;
> 
> llvm::errs().

Done

> Index: utils/obj2yaml/CMakeLists.txt
> ===================================================================
> --- utils/obj2yaml/CMakeLists.txt	(revision 0)
> +++ utils/obj2yaml/CMakeLists.txt	(revision 0)
> @@ -0,0 +1,7 @@
> +set(LLVM_LINK_COMPONENTS archive object)
> 
> You don't actually need to link to archive here. That's the "old"
> archive library.

OK.

> Index: utils/obj2yaml/Makefile
> ===================================================================
> --- utils/obj2yaml/Makefile	(revision 0)
> +++ utils/obj2yaml/Makefile	(revision 0)
> @@ -0,0 +1,20 @@
> +##===- utils/obj2yaml/Makefile ----------------------------*- Makefile -*-===##
> +#
> +#                     The LLVM Compiler Infrastructure
> +#
> +# This file is distributed under the University of Illinois Open Source
> +# License. See LICENSE.TXT for details.
> +#
> +##===----------------------------------------------------------------------===##
> +
> +LEVEL = ../..
> +TOOLNAME = obj2yaml
> +USEDLIBS = LLVMSupport.a
> 
> This should be
> 
> LINK_COMPONENTS := object
> 
> Instead. As it is in llvm-nm.

OK.

> There are still lots of cases where you have extra spaces around ( and ).


I think I got all of them.

-- Marshall

Marshall Clow     Idio Software   <mailto:mclow.lists at gmail.com>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
        -- Yu Suzuki
-------------- next part --------------
A non-text attachment was scrubbed...
Name: obj2yaml-3.patch
Type: application/octet-stream
Size: 19489 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120618/e8b12932/attachment.obj>