This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: gold patch for fixing ICF bugs.
- From: Cary Coutant <ccoutant at google dot com>
- To: Sriraman Tallam <tmsriram at google dot com>
- Cc: binutils <binutils at sourceware dot org>, Ian Lance Taylor <iant at google dot com>
- Date: Mon, 19 Apr 2010 16:36:43 -0700
- Subject: Re: gold patch for fixing ICF bugs.
- References: <w2u863b0cbf1004181652l84316fc3zfc23bd261a0f2e3f@mail.gmail.com>
> ? ? ? ?* icf.cc (get_section_contents): Check for preemptible functions.
> ? ? ? ?Ignore addend when appropriate.
> ? ? ? ?* symtab.cc (should_add_dynsym_entry): Add new parameter. ?Check for
> ? ? ? ?section folded.
> ? ? ? ?(add_from_relobj): Check for section folded.
> ? ? ? ?(set_dynsym_indexes): Fix call to should_add_dynsym_entry.
> ? ? ? ?* symtab.h (should_add_dynsym_entry): Add new parameter.
> ? ? ? ?* target-reloc.h (scan_relocs): Check for section folded.
> ? ? ? ?* x86_64.cc (Target_x86_64::Scan::possible_function_pointer_reloc):
> ? ? ? ?Check reloc types for function pointers in shared objects.
> ? ? ? ?* testsuite/Makefile.am (icf_virtual_function_folding_test): New test
> ? ? ? ?case.
> ? ? ? ?(icf_preemptible_functions_test): New test case.
> ? ? ? ?(icf_string_merge_test): New test case.
> ? ? ? ?* testsuite.Makefile.in: Regenerate.
> ? ? ? ?* testsuite/icf_safe_so_test.sh: Change to not fold foo_glob and
> ? ? ? ?bar_glob. ?Refactor code.
> ? ? ? ?* testsuite/icf_preemptible_functions_test.cc: New file.
> ? ? ? ?* testsuite/icf_preemptible_functions_test.sh: New file.
> ? ? ? ?* testsuite/icf_string_merge_test.cc: New file.
> ? ? ? ?* testsuite/icf_string_merge_test.sh: New file.
> ? ? ? ?* testsuite/icf_virtual_function_folding_test.cc: New file.
> ? ? ? ?* testsuite/icf_virtual_function_folding_test.sh: New file.
It looks good to me; I just have a nit on one comment:
+ long long offset = it_a->first;
+
+ unsigned long long addend = it_a->second;
+ // Ignoring the addend when it is a negative value --
+ // this is taken from function 'Value' in file 'object.h'.
+ if (addend < 0xffffff00)
+ offset = offset + it_a->second;
The comment should be a complete sentence: "Ignore the addend when
...". Instead of saying "this is taken from", it would be better to
say "See the comments for Merged_symbol_value::value() in object.h."
-cary