public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jan Hubicka <hubicka@ucw.cz>
To: Eric Botcazou <ebotcazou@adacore.com>
Cc: Jan Hubicka <hubicka@ucw.cz>, gcc-patches@gcc.gnu.org
Subject: Re: Fix lto-symtab ICE during Ada LTO bootstrap
Date: Sun, 22 Nov 2015 18:49:00 -0000	[thread overview]
Message-ID: <20151122184131.GA85454@kam.mff.cuni.cz> (raw)
In-Reply-To: <1690140.OfB3ATH0FC@polaris>

> > this patch fixes an ICE seen with Ada LTO bootstrap in reporting type
> > mismatches and it also makes us to stop complaining about C++ ODR
> > violation.  The warnings are however correct.  I looked at few:
> > 
> > ../../libiberty/xstrerror.c:40:14: warning: type of �strerror� does not
> > match original declaration [-Wlto-type-mismatch] extern char *strerror
> > (int);
> >               ^
> > 
> > ../../gcc/ada/s-os_lib.adb:1007:16: note: return value type mismatch
> >        function strerror (errnum : Integer) return System.Address;
> >                 ^
> > 
> > ../../gcc/ada/s-os_lib.adb:1007:16: note:
> > �system__os_lib__errno_message__strerror� was previously declared here
> > 
> > Here we have function returning pointer WRT function returning integer:
> 
> This one is on purpose and cannot be easily changed.  Pointer types (or access 
> types as called in Ada) are avoided as much as possible in the runtime because 
> they drag the accessibility machinery, which is the machinery present in the 
> language to eliminate dangling references and is heavy; so they are usually 
> imported as System.Address instead.

Yep, I read the specification yesterday.

It seems to me that the only way to handle this is to put the integer type
used to represent pointers to the same alias set as void_ptr_type.
This is doable by simply making gimple_canonical_types_compatible_p and
lto.c handle this type specially as "pointer".

I can implement this; but I woul dlike to enable this kind of globing only
when there is an union in Ada.

> 
> > <built-in>: warning: type of �__builtin_strlen� does not match original
> > declaration [-Wlto-type-mismatch] ../../gcc/ada/osint.adb:422:19: note:
> > return value type mismatch
> > ../../gcc/ada/osint.adb:422:19: note: type �integer� should match type �long
> > unsigned int�
> > 
> > Here the signedness of integer does not match:
> 
> Yes, it's clearly incorrect on the Ada side and should be fixed in osint.adb.

Great!
> 
> > ../../gcc/ada/s-os_lib.ads:1053:4: warning: type of
> > �system__os_lib__directory_separator� does not match original declaration
> > [-Wlto-type-mismatch] Directory_Separator : constant Character;
> >     ^
> > 
> > ../../gcc/ada/adaint.c:225:6: note: type �char� should match type �volatile
> > character� char __gnat_dir_separator = DIR_SEPARATOR;
> >       ^
> > 
> > Here we get difference in  signedness and volatility:
> 
> The signedness issue for Character is known and we plan to address it; the 
> volatility issue was overlooked but looks fixable too.

Actually the volatility is not a big deal: we ignore it for canonical type
computation anyway (probably we don't, but in general we do not need
to care much about optimizing volatile memory accesses, so we lose nothing by
handling this safely)
> 
> > All those types will lead to wrong code if ever written to same memory
> > location because of TBAA.  Eric, does Ada need all this types to be TBAA
> > compatible? If so, we need to implement more strict globbing as we did for
> > Fortran and we probably finally need to make the globbing aware of
> > languages involved (we definitly don't want to glob pointers and integers
> > for C/C++ programs)
> 
> I think that we can fix all the problems on the Ada side except for the 
> pointer/System.Address duality (which can be even more problematic on 
> architectures that use different calling conventions for them).

That sounds good.  I can try to cook up the prototype patch for pointers.
I fixed issues that breaks Ada bootstrap, but I still get an ICE in LTO
built gnat1 in Ada code.  I am not quite sure what is wrong (it fails
early setting some flags in atree, but that is as far as I can read the code)

Honza
> 
> > Eric, it would be great to have a stand alone testcases in style of
> > gcc/testsuite/gfortran.dg/lto/bind_c-*
> > which stores and reads the same memory location in same alias set and thus
> > trigger the undefined behvaiour.
> 
> OK, I'll think about that, thanks.
> 
> -- 
> Eric Botcazou

      parent reply	other threads:[~2015-11-22 18:41 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-21 18:35 Jan Hubicka
2015-11-22 14:46 ` Eric Botcazou
2015-11-22 15:17   ` Arnaud Charlet
2015-11-23  1:24     ` Jan Hubicka
2015-11-23  9:59       ` Arnaud Charlet
2015-11-23 11:13         ` Eric Botcazou
2015-11-23 11:24           ` Arnaud Charlet
2015-11-23 12:00             ` Eric Botcazou
2015-11-23 13:35               ` Richard Biener
2015-11-23 16:05                 ` Eric Botcazou
2015-11-23 16:17                   ` H.J. Lu
2015-11-23 16:26                     ` Eric Botcazou
2015-11-23 16:31                       ` Arnaud Charlet
2015-11-23 19:05                       ` Jan Hubicka
2015-11-23 22:29                         ` Eric Botcazou
2015-11-23 22:53                           ` Jan Hubicka
2015-11-23 23:09                             ` Jan Hubicka
2015-12-20  6:54                         ` Jan Hubicka
2015-12-20  8:14                           ` Eric Botcazou
2015-12-20 22:20                         ` Eric Botcazou
2015-12-21 10:20                           ` Eric Botcazou
2015-12-21 14:19                             ` Jan Hubicka
2015-12-21 15:16                               ` Eric Botcazou
2015-12-21 14:20                             ` Jan Hubicka
2015-11-22 18:49   ` Jan Hubicka [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151122184131.GA85454@kam.mff.cuni.cz \
    --to=hubicka@ucw.cz \
    --cc=ebotcazou@adacore.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).