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: [RFC PATCH] diagnose built-in declarations without prototype (PR 83656)


On 07/03/2018 08:33 PM, Jeff Law wrote:
On 06/29/2018 09:54 AM, Martin Sebor wrote:

All the warnings I have seen are because of declarations like
the one in the test below that checks for the presence of symbol
sin in the library:

  char sin ();
  int main () { return sin (); }

GCC has warned for this code by default since at least 4.1 so if
it is, in fact, a problem it has been with us for over a decade.

There's a comment in the test that explains that the char return
type is deliberate to prevent GCC from treating the function as
a built-in.  (That, of course, relies on undefined behavior
because names of extern library functions are reserved and
conforming compilers could simply avoid emitting the call or
replace it with a trap.)
As you noted, by doing this the test can verify if there's a sin()
function in the library or not, regardless of whether or not the
compiler has a builtin for sin().
I wonder if stepping forward to a more modern version of autoconf is
going to help here and if we should be feeding them updates to make this
kind of stuff less pervasive, at least in standard autoconf tests.

That would make sense to me.  The tests should not rely on
undefined behavior.  They should declare standard functions with
the right prototypes.  IMO, for GCC and compatible compilers they
should disable built-in expansion instead via -fno-builtin.  For
all other compilers, they could store the address of each function
in a (perhaps volatile) pointer and use it to make the call instead.
I think the problem is they can't rely on the compiler having the
-fno-builtin flag.  Of course they're relying on other compilers having
the same kind of behavior as GCC WRT which is possibly worse.

I guess there's a reason why they didn't extract the set of symbols from
the library. :-)


But since the number of warnings here hasn't changed, the ones
in GCC logs predate my changes.  So updating the tests seems
like an improvement to consider independently of the patch.
Agreed.  I'm still wary of proceeding given the general concerns about
configure tests.  It's good that GCC's configury bits aren't affected,
but I'm not sure we can generalize a whole lot from that.

So what's the next step?  I'm open to relaxing the warning
so it only triggers with -Wall or -Wextra and not by default
if that's considered necessary.

At the same time, the instances of the warning we have seen
have all been issued for the configure tests for years and
we have not seen any new instances of it as a result of
this change, so the concern that the patch might lead to some
more while at the same time accepting the ones we know about
doesn't make sense to me.

Any new warning that's enabled by default has the potential
to trigger for invalid configure tests or any other such code.
I would expect tests deliberately written to rely on undefined
behavior to be prepared for compiler warnings, so I'm not sure
I understand the basis for the concern in this case.  I also
don't recall the concern being raised for newly added warnings
in the past that were enabled by default and I'm not sure I see
what makes this one different.

Martin


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