[PATCH] D14129: ELF2: Make .gnu.hash default instead of .hash.
Rui Ueyama via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 27 14:01:10 PDT 2015
ruiu added inline comments.
================
Comment at: ELF/Config.h:63
@@ -63,2 +62,3 @@
+ bool SysvHash = false;
bool Verbose;
bool ZNodelete = false;
----------------
grimar wrote:
> May be it worth to rename these 2 to be placed together like HashGnu and HashSysv ? Or just make a single enum ?
GnuHash and SysvHash sound natural to me, so I'll leave them alone.
================
Comment at: ELF/Driver.cpp:176
@@ -175,3 +175,3 @@
error("Unknown hash style: " + S);
}
----------------
grimar wrote:
> May be it worth to explicit set flags for "both" ?
>
>
> if (S == "both") {
> Config->SysvHash = true;
> Config->GnuHash = true;
> }
>
>
> it makes it more clear (I saw this first time and was need to look into Configuration to check that both works as expected + without that it depends on default initialization which can change).
>
That is probably excessive. There are actually five cases: gnu, sysv, both, invalid argument, or no --hash-style flag. We cover four cases here. No matter how we write code for "both", the last one is pretty much implicit.
================
Comment at: test/elf2/discard-none.s:49
@@ -49,2 +48,3 @@
+// CHECK-NEXT: Value: 0x154
// CHECK-NEXT: Size: 0
// CHECK-NEXT: Binding: Local
----------------
grimar wrote:
> Do we really need to mantain adresses in such cases ? They changes often, may be we should just skip checking here like:
> // CHECK-NEXT: Value: 0x
> ?
I feel like this serves the purpose as a safeguard. Not sure how effective that is, but I don't think removing that is the right thing to do in this patch.
================
Comment at: test/elf2/shared.s:284
@@ -283,10 +283,3 @@
-// CHECK: HashTable {
-// CHECK-NEXT: Num Buckets: 4
-// CHECK-NEXT: Num Chains: 4
-// CHECK-NEXT: Buckets: [3, 0, 2, 0]
-// CHECK-NEXT: Chains: [0, 0, 0, 1]
-// CHECK-NEXT: }
-
.global _start
_start:
----------------
grimar wrote:
> Was the next part of test removed intentionaly ?
>
> // CHECK: HashTable {
> // CHECK-NEXT: Num Buckets: 4
> // CHECK-NEXT: Num Chains: 4
> // CHECK-NEXT: Buckets: [3, 0, 2, 0]
> // CHECK-NEXT: Chains: [0, 0, 0, 1]
> // CHECK-NEXT: }
This is intentional.
http://reviews.llvm.org/D14129
More information about the llvm-commits
mailing list