"Jose E. Marchesi" <jose.march...@oracle.com> writes:
> Hi Richard.
> Thanks for looking at this! :)
>
>
>> "Jose E. Marchesi" <jose.march...@oracle.com> writes:
>>> ping
>>
>> I don't know this code very well, and have AFAIR haven't worked
>> with an assembler that requires external declarations, but since
>> it's at a second ping :)
>>
>>>
>>>> ping
>>>>
>>>>> [Differences from V1:
>>>>> - Prototype for call_from_call_insn moved before comment block.
>>>>> - Reuse the `call' flag for SYMBOL_REF_LIBCALL.
>>>>> - Fallback to check REG_CALL_DECL in non-direct calls.
>>>>> - New test to check correct behavior for non-direct calls.]
>>>>>
>>>>> There are many places in GCC where alternative local sequences are
>>>>> tried in order to determine what is the cheapest or best alternative
>>>>> to use in the current target. When any of these sequences involve a
>>>>> libcall, the current implementation of emit_library_call_value_1
>>>>> introduce a side-effect consisting on emitting an external declaration
>>>>> for the funcall (such as __divdi3) which is thus emitted even if the
>>>>> sequence that does the libcall is not retained.
>>>>>
>>>>> This is problematic in targets such as BPF, because the kernel loader
>>>>> chokes on the spurious symbol __divdi3 and makes the resulting BPF
>>>>> object unloadable. Note that BPF objects are not linked before being
>>>>> loaded.
>>>>>
>>>>> This patch changes emit_library_call_value_1 to mark the target
>>>>> SYMBOL_REF as a libcall. Then, the emission of the external
>>>>> declaration is done in the first loop of final.cc:shorten_branches.
>>>>> This happens only if the corresponding sequence has been kept.
>>>>>
>>>>> Regtested in x86_64-linux-gnu.
>>>>> Tested with host x86_64-linux-gnu with target bpf-unknown-none.
>>
>> I'm not sure that shorten_branches is a natural place to do this.
>> It isn't something that would normally emit asm text.
>
> Well, that was the approach suggested by another reviewer (Jakub) once
> my initial approach (in the V1) got rejected. He explicitly suggested
> to use shorten_branches.
>
>> Would it be OK to emit the declaration at the same point as for decls,
>> which IIUC is process_pending_assemble_externals? If so, how about
>> making assemble_external_libcall add the symbol to a list when
>> !SYMBOL_REF_USED, instead of calling targetm.asm_out.external_libcall
>> directly? assemble_external_libcall could then also call get_identifier
>> on the name (perhaps after calling strip_name_encoding -- can't
>> remember whether assemble_external_libcall sees the encoded or
>> unencoded name).
>>
>> All being well, the call to get_identifier should cause
>> assemble_name_resolve to record when the name is used, via
>> TREE_SYMBOL_REFERENCED. Then process_pending_assemble_externals could
>> go through the list of libcalls recorded by assemble_external_libcall
>> and check whether TREE_SYMBOL_REFERENCED is set on the get_identifier.
>>
>> Not super elegant, but it seems to fit within the existing scheme.
>> And I don't there should be any problem with using get_identifier
>> for libcalls, since it isn't valid to use libcall names for other
>> types of symbol.
>
> This sounds way more complicated to me than the approach in V2, which
> seems to work and is thus a clear improvement compared to the current
> situation in the trunk. The approach in V2 may be ugly, but it is
> simple and easy to understand. Is the proposed more convoluted
> alternative really worth the extra complexity, given it is "not super
> elegant"?
Is it really that much more convoluted? I was thinking of something
like the attached, which seems a bit shorter than V2, and does seem
to fix the bpf tests.
I think most (all?) libcalls already have an associated decl due to
optabs-libfuncs.cc, so an alternative to get_identifier would be to
set the SYMBOL_REF_DECL. Using get_identiifer seems a bit more
lightweight though.
Richard
diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index b0eff17b8b5..073e3eb2579 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -2461,6 +2461,10 @@ contains_pointers_p (tree type)
it all the way to final. See PR 17982 for further discussion. */
static GTY(()) tree pending_assemble_externals;
+/* A similar list of pending libcall symbols. We only want to declare
+ symbols that are actually used in the final assembly. */
+static GTY(()) rtx pending_libcall_symbols;
+
#ifdef ASM_OUTPUT_EXTERNAL
/* Some targets delay some output to final using TARGET_ASM_FILE_END.
As a result, assemble_external can be called after the list of externals
@@ -2516,12 +2520,20 @@ void
process_pending_assemble_externals (void)
{
#ifdef ASM_OUTPUT_EXTERNAL
- tree list;
- for (list = pending_assemble_externals; list; list = TREE_CHAIN (list))
+ for (tree list = pending_assemble_externals; list; list = TREE_CHAIN (list))
assemble_external_real (TREE_VALUE (list));
+ for (rtx list = pending_libcall_symbols; list; list = XEXP (list, 1))
+ {
+ rtx symbol = XEXP (list, 0);
+ tree id = get_identifier (XSTR (symbol, 0));
+ if (TREE_SYMBOL_REFERENCED (id))
+ targetm.asm_out.external_libcall (symbol);
+ }
+
pending_assemble_externals = 0;
pending_assemble_externals_processed = true;
+ pending_libcall_symbols = NULL_RTX;
delete pending_assemble_externals_set;
#endif
}
@@ -2594,8 +2606,11 @@ assemble_external_libcall (rtx fun)
/* Declare library function name external when first used, if nec. */
if (! SYMBOL_REF_USED (fun))
{
+ gcc_assert (!pending_assemble_externals_processed);
SYMBOL_REF_USED (fun) = 1;
- targetm.asm_out.external_libcall (fun);
+ get_identifier (XSTR (fun, 0));
+ pending_libcall_symbols = gen_rtx_EXPR_LIST (VOIDmode, fun,
+ pending_libcall_symbols);
}
}
--
2.25.1
