This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PATCH: PR middle-end/20297: #pragma GCC visibility isn't properlyhandled for builtin functions


On Thu, 3 Mar 2005, H. J. Lu wrote:
> > Can you explain exactly how this helps?  expand_builtin is called
> > in the middle-end during RTL expansion for calls *to* intrinsic
> > functions.  I don't understand how this could possibly affect the
> > visibility of a "memcpy" symbol.  expand_builtin doesn't emit any
> > symbols, and often doesn't reference any.
> >
> expand_builtin will emit a library call when it fails to inline the
> intrinsic function. In this case, it will emit a call to memcpy.
>
> > Isn't "pragma visibility" a front-end concept that should be attached
> > to DECLs?
>
> When expand_builtin emits a libray call, it uses the current
> "pragma visibility", which may be different from the "pragma
> visibility" of the intrinsic function which it failed to inline. That
> is not what the source code says.


I think that the current implementation strategy for declaration
visibility is fundamentally broken.


http://gcc.gnu.org/ml/gcc-patches/2004-07/txt00076.txt
This change above modified the implementation of build_decl in tree.c,
which is a low-level front-end independent function used thoughout
the compiler create DECL_EXPRs.  This is used by the compiler
to create DECLs for libfuncs, standard library calls, temporary
variables, vtables, their guard variables, etc...

The change above now makes the behaviour of build_decl dependent
upon the current parser location in the input stream.  Hence,
many of the middle-end's decls are randomly being corrupted
based upon whether the generation of RTL for them has been deferred
or not, and which point in a function it decides/requires
creation of a temporary decl.  We may even be generating different
code in unit-at-a-times vs. default vs. IMA? processing modes.



    /* Set default visibility to whatever the user supplied with
       visibility_specified depending on #pragma GCC visibility.  */
    DECL_VISIBILITY (t) = default_visibility;
    DECL_VISIBILITY_SPECIFIED (t) = visibility_options.inpragma;


So for example, the following code in expr.c is now badly
non-deterministic behaviour due to the above hunk in build_decl:

void
init_block_move_fn (const char *asmspec)
{
  if (!block_move_fn)
    {
      tree args, fn;

      fn = get_identifier ("memcpy");
      args = build_function_type_list (ptr_type_node, ptr_type_node,
                                       const_ptr_type_node, sizetype,
                                       NULL_TREE);

      fn = build_decl (FUNCTION_DECL, fn, args);
      DECL_EXTERNAL (fn) = 1;
      TREE_PUBLIC (fn) = 1;
      DECL_ARTIFICIAL (fn) = 1;
      TREE_NOTHROW (fn) = 1;

      block_move_fn = fn;
    }


Note that your suggested change to expand_builtin only works around
a minor aspect of the problem.  For example, the above init_block_move_fn
is used by the middle-end even when the original source neither calls
nor prototypes memcpy, for example in C to assign one a large structure
to another.  In this case, GCC's expand_builtin is never even called.


I suspect that a far more appropriate (and less broken) implementation
of visibility attributes, is not to change tree.c's build_decl and thereby
affect every call to it throughout the compiler, but instead to handle
these attributes in the front-ends, and identify the two or three places
where *user* functions and global variables are defined in the C/C++
front-ends, and appropriately set the flag bits on the DECL created by
build_decl.


I'm shocked at how broken things currently are.


Alternatively, rather than place your ugly work-around in the
innocent expand_builtin function, you could instead place a similar
hack in init_block_move_fn above, to reset DECL_VISIBILITY and
DECL_VISIBILITY_SPECIFIED after the call to build_decl.  Be warned
though, if you go this route, these flags probably need to be reset
after every call build_decl in the middle-end, and the alpha, i386,
i860, iq2000, mips, rs6000, s390, sh, stormy16 and xtensa backends.


This, of course, is just my opinion, and other GCC maintainers might
disagree. Due to the changes required to front-end files, I suspect
it may even require a global maintainer to sort this mess out.

Roger
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]