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

[Nick Lewycky]

Marshall Clow wrote:
> 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.

For reference, this is http://llvm.org/docs/CodingStandards.html#ll_end 
in the Coding Standards. I realize that it may not matter in this case, 
but we still prefer it for consistency.

Nick

>
>
>> +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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits