> "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.
o_O Ok I clearly misunderstood what you was proposing. This is way simpler! How does the magic of TREE_SYMBOL_REFERENCED work? How is it set to `true' only if the RTL containing the call is retained in the final chain? > 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); > } > }