[PATCH] [Object] Split the ELF interface into 3 parts.
Michael Spencer
bigcheesegs at gmail.com
Wed Jul 31 12:18:09 PDT 2013
================
Comment at: include/llvm/Object/ELF.h:113
@@ -553,1 +112,3 @@
+ uintX_t getEntSize() const { return EntitySize; }
+
----------------
Rafael Ávila de Espíndola wrote:
> nit: Which entities can be bigger than 4GB each? Maybe just using unsigned for EntitySize would do.
>
Probably none, but I don't see why we should truncate unless there's a good reason. This class is going to be 16 bytes on a 64 bit system anyway.
================
Comment at: include/llvm/Object/ELF.h:365
@@ -786,4 +364,3 @@
- Elf_Rel_Iter endELFRel(const Elf_Shdr *sec) const {
- return Elf_Rel_Iter(sec->sh_entsize, (const char *)
- (base() + sec->sh_offset + sec->sh_size));
+ Elf_Rel_Iter end_rel(const Elf_Shdr *sec) const {
+ return Elf_Rel_Iter(sec->sh_entsize,
----------------
Rafael Ávila de Espíndola wrote:
> Having ELF in the name of this methods would be redundant, so I agree with dropping it, but why the switch away from CamelCase?
Because all the other iterator functions are like this.
================
Comment at: include/llvm/Object/ELF.h:386
@@ -815,3 +385,3 @@
uint64_t getNumSections() const;
- uint64_t getStringTableIndex() const;
+ uintX_t getStringTableIndex() const;
ELF::Elf64_Word getSymbolTableIndex(const Elf_Sym *symb) const;
----------------
Rafael Ávila de Espíndola wrote:
> Nit: maybe an unsigned would do? With 64 bytes per section, the file would be 256 GB even if all 2^32 sections were empty.
>
It's the size of the value in the file. I don't see a point in truncating.
================
Comment at: include/llvm/Object/ELF.h:1405
@@ +1404,3 @@
+ // stripped.
+#if 0
+ for (Elf_Dyn_Iter DynI = begin_dynamic_table(), DynE = end_dynamic_table();
----------------
Rafael Ávila de Espíndola wrote:
> no #if 0 code please.
k
================
Comment at: lib/Object/ELFObjectFile.cpp:18
@@ -17,1 +17,3 @@
namespace llvm {
+namespace object {
+#define LLVM_ELF_SWITCH_RELOC_TYPE_NAME(enum) \
----------------
Rafael Ávila de Espíndola wrote:
> This is now duplicated from the header, no? I would be really nice to keep just one copy out of header for this!
>
Ah, didn't realize it was duplicated.
================
Comment at: test/Object/corrupt.test:1
@@ +1,2 @@
+RUN: llvm-readobj %p/Inputs/corrupt.elf-x86-64 -sections \
+RUN: 2>&1 | FileCheck %s
----------------
Rafael Ávila de Espíndola wrote:
> llvm-readobject is not returning an error code? You will need to add a not before llvm-readobj now that we are using pipefail.
>
> Please document in which way this file is corrupt. Please make sure we have corrupt tests for:
>
> * getSymbolName
> * getSymbolVersion
> * getSectionName
> * getSectionContents
>
k
http://llvm-reviews.chandlerc.com/D1033
More information about the llvm-commits
mailing list