[ARM] PR 78253 do not resolve weak ref locally

Message ID CAKdteOY57XHHGSKxTuWesTypp7e-8HS6HBLqcvP50FEssQfqcw@mail.gmail.com
State New
Headers show

Commit Message

Christophe Lyon Nov. 9, 2016, 9:29 p.m.
Hi,

PR 78253 shows that the handling of weak references has changed for
ARM with gcc-5.

When r220674 was committed, default_binds_local_p_2 gained a new
parameter (weak_dominate), which, when true, implies that a reference
to a weak symbol defined locally will be resolved locally, even though
it could be overridden by a strong definition in another object file.

With r220674, default_binds_local_p forces weak_dominate=true,
effectively changing the previous behavior.

The attached patch introduces default_binds_local_p_4 which is a copy
of default_binds_local_p_2, but using weak_dominate=false, and updates
the ARM target to call default_binds_local_p_4 instead of
default_binds_local_p_2.

I ran cross-tests on various arm* configurations with no regression,
and checked that the test attached to the original bugzilla now works
as expected.

I am not sure why weak_dominate defaults to true, and I couldn't
really understand why by reading the threads related to r220674 and
following updates to default_binds_local_p_* which all deal with other
corner cases and do not discuss the weak_dominate parameter.

Or should this patch be made more generic?

Thanks,

Christophe
2016-11-09  Christophe Lyon  <christophe.lyon@linaro.org>

	PR target/78253
	* output.h (default_binds_local_p_4): New.
	* varasm.c (default_binds_local_p_4): New, use
	weak_dominate=false.
	* config/arm/linux-elf.h (TARGET_BINDS_LOCAL_P): Define to
	default_binds_local_p_4.

Comments

Richard Earnshaw Nov. 10, 2016, 10:05 a.m. | #1
On 09/11/16 21:29, Christophe Lyon wrote:
> Hi,

> 

> PR 78253 shows that the handling of weak references has changed for

> ARM with gcc-5.

> 

> When r220674 was committed, default_binds_local_p_2 gained a new

> parameter (weak_dominate), which, when true, implies that a reference

> to a weak symbol defined locally will be resolved locally, even though

> it could be overridden by a strong definition in another object file.

> 

> With r220674, default_binds_local_p forces weak_dominate=true,

> effectively changing the previous behavior.

> 

> The attached patch introduces default_binds_local_p_4 which is a copy

> of default_binds_local_p_2, but using weak_dominate=false, and updates

> the ARM target to call default_binds_local_p_4 instead of

> default_binds_local_p_2.

> 

> I ran cross-tests on various arm* configurations with no regression,

> and checked that the test attached to the original bugzilla now works

> as expected.

> 

> I am not sure why weak_dominate defaults to true, and I couldn't

> really understand why by reading the threads related to r220674 and

> following updates to default_binds_local_p_* which all deal with other

> corner cases and do not discuss the weak_dominate parameter.

> 

> Or should this patch be made more generic?

> 


I certainly don't think it should be ARM specific.

The questions I have are:

1) What do other targets do today.  Are they the same, or different?
2) If different why?
3) Is the current behaviour really what was intended by the patch?  ie.
Was the old behaviour actually wrong?

R.
> Thanks,

> 

> Christophe

>
Christophe Lyon Nov. 10, 2016, 2:10 p.m. | #2
On 10 November 2016 at 11:05, Richard Earnshaw
<Richard.Earnshaw@foss.arm.com> wrote:
> On 09/11/16 21:29, Christophe Lyon wrote:

>> Hi,

>>

>> PR 78253 shows that the handling of weak references has changed for

>> ARM with gcc-5.

>>

>> When r220674 was committed, default_binds_local_p_2 gained a new

>> parameter (weak_dominate), which, when true, implies that a reference

>> to a weak symbol defined locally will be resolved locally, even though

>> it could be overridden by a strong definition in another object file.

>>

>> With r220674, default_binds_local_p forces weak_dominate=true,

>> effectively changing the previous behavior.

>>

>> The attached patch introduces default_binds_local_p_4 which is a copy

>> of default_binds_local_p_2, but using weak_dominate=false, and updates

>> the ARM target to call default_binds_local_p_4 instead of

>> default_binds_local_p_2.

>>

>> I ran cross-tests on various arm* configurations with no regression,

>> and checked that the test attached to the original bugzilla now works

>> as expected.

>>

>> I am not sure why weak_dominate defaults to true, and I couldn't

>> really understand why by reading the threads related to r220674 and

>> following updates to default_binds_local_p_* which all deal with other

>> corner cases and do not discuss the weak_dominate parameter.

>>

>> Or should this patch be made more generic?

>>

>

> I certainly don't think it should be ARM specific.

That was my feeling too.

>

> The questions I have are:

>

> 1) What do other targets do today.  Are they the same, or different?


arm, aarch64, s390 use default_binds_local_p_2 since PR 65780, and
default_binds_local_p before that. Both have weak_dominate=true
i386 has its own version, calling default_binds_local_p_3 with true
for weak_dominate

But the behaviour of default_binds_local_p changed with r220674 as I said above.
See https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=220674 and
notice how weak_dominate was introduced

The original bug report is about a different case:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=32219

The original patch submission is
https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00410.html
and the 1st version with weak_dominate is in:
https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00469.html
but it's not clear to me why this was introduced

> 2) If different why?

on aarch64, although binds_local_p returns true, the relocations used when
building the function pointer is still the same (still via the GOT).

aarch64 has different logic than arm when accessing a symbol
(eg aarch64_classify_symbol)

> 3) Is the current behaviour really what was intended by the patch?  ie.

> Was the old behaviour actually wrong?

>

That's what I was wondering.
Before r220674, calling a weak function directly or via a function
pointer had the same effect (in other words, the function pointer
points to the actual implementation: the strong one if any, the weak
one otherwise).

After r220674, on arm the function pointer points to the weak
definition, which seems wrong to me, it should leave the actual
resolution to the linker.


> R.

>> Thanks,

>>

>> Christophe

>>

>

Patch hide | download patch | download mbox

diff --git a/gcc/config/arm/linux-elf.h b/gcc/config/arm/linux-elf.h
index cc17b51..4f32ce8 100644
--- a/gcc/config/arm/linux-elf.h
+++ b/gcc/config/arm/linux-elf.h
@@ -110,7 +110,7 @@ 
    strong definitions in dependent shared libraries, will resolve
    to COPY relocated symbol in the executable.  See PR65780.  */
 #undef TARGET_BINDS_LOCAL_P
-#define TARGET_BINDS_LOCAL_P default_binds_local_p_2
+#define TARGET_BINDS_LOCAL_P default_binds_local_p_4
 
 /* Define this to be nonzero if static stack checking is supported.  */
 #define STACK_CHECK_STATIC_BUILTIN 1
diff --git a/gcc/output.h b/gcc/output.h
index 0924499..11b5ce5 100644
--- a/gcc/output.h
+++ b/gcc/output.h
@@ -585,6 +585,7 @@  extern bool default_binds_local_p (const_tree);
 extern bool default_binds_local_p_1 (const_tree, int);
 extern bool default_binds_local_p_2 (const_tree);
 extern bool default_binds_local_p_3 (const_tree, bool, bool, bool, bool);
+extern bool default_binds_local_p_4 (const_tree);
 extern void default_globalize_label (FILE *, const char *);
 extern void default_globalize_decl_name (FILE *, tree);
 extern void default_emit_unwind_label (FILE *, tree, int, int);
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 6a7ffc2..7a3cf99 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6952,6 +6952,16 @@  default_binds_local_p_2 (const_tree exp)
 				  !flag_pic);
 }
 
+/* Similar to default_binds_local_p_2, but local weak definition does
+   not imply local resolution (weak_dominate is false).  */
+
+bool
+default_binds_local_p_4 (const_tree exp)
+{
+  return default_binds_local_p_3 (exp, flag_shlib != 0, false, true,
+				  !flag_pic);
+}
+
 bool
 default_binds_local_p_1 (const_tree exp, int shlib)
 {