[llvm-commits] PATCH: add new test tool: obj2yaml
Nick Lewycky
nicholas at mxc.ca
Mon Jun 18 21:12:00 PDT 2012
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
More information about the llvm-commits
mailing list