Re: [PATCH 0/6] v2 of libdiagnostics
Hi David, On 2023-11-21 22:20, David Malcolm wrote: > Here's v2 of the "libdiagnostics" shared library idea; see: > https://gcc.gnu.org/wiki/libdiagnostics > > As in v1, patch 1 (for GCC) shows libdiagnostic.h (the public > header file), along with examples of simple self-contained programs that > show various uses of the API. > > As in v1, patch 2 (for GCC) is the work-in-progress implementation. > > Patch 3 (for GCC) adds a new libdiagnostics++.h, a wrapper API providing > some syntactic sugar when using the API from C++. I've been using this > to "eat my own dogfood" and write a simple SARIF-dumping tool: > https://github.com/davidmalcolm/libdiagnostics-sarif-dump > > Patch 4 (for GCC) is an internal change needed by patch 1. > > Patch 5 (for GCC) updates GCC's source printing code so that when > there's no column information, we don't print annotation lines. This > fixes the extra lines seen using it from gas discussed in: > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635575.html > > Patch 6 (for binutils) is an updated version of the experiment at using > the API from gas. > > Thoughts? Do you have plans on making this a top level library instead? That would allow easily making it a non-optional dependency for binutils, as we could have the library in the binutils-gdb repo as well, for instance. From the Cauldron discussion I understood that the diagnostics stuff doesn't depend on much of GCC's data structures, and doesn't rely on the garbage collector. Is there something preventing that? (Other than "it's-a-matter-of-time/effort", of course.) Pedro Alves
Re: [PATCH 1/2] Add gcc/make-unique.h
On 2022-07-12 7:50 p.m., Jonathan Wakely wrote: > Yeah, and I don't think optimizing for indentation is the right trade off. > Putting something in a namespace with a three-letter name just so you don't > have to re-indent some statements in a few years seems odd. > > I see no technical difficulty replacing a custom make_unique (in any > namespace) with std::make_unique at a later date. > > If a namespace made sense to avoid name clashes, or to enable finding > functions by ADL, or other technical reasons, I'd be all for it. But no such > rationale was given, and using a namespace for indentation just seems odd to > me. > > But I really don't care. Putting it in the global namespace or a 'gcc' > namespace or anything else appropriate to GCC is fine. I'll leave this thread > now. I don't think this is very constructive and I'm sorry for objecting to > the suggestion. That's fair. Cheers! Hope we'll be able to laugh about it in Prague!
Re: [PATCH 1/2] Add gcc/make-unique.h
On 2022-07-12 7:36 p.m., David Malcolm wrote: > On Tue, 2022-07-12 at 17:40 +0100, Pedro Alves wrote: >>> >> >> If that's the approach, then GCC should import std::unique_ptr, >> std::move, >> std::foo, std::bar into the gcc namespace too, no? Are you really >> going >> to propose that? > > Pedro, it feels to me like you're constructing a strawman here. > Neither me nor Jonathan are proposing that. > See my other reply. I am actually appalled at how the conversion seems to have derailed. > I just want to be able to comfortably use std::unique_ptr in GCC in the > places for which it makes sense, and being able to use "make_unique" is > a part of that. My suggestion was simple: wrap your make_unique in namespace gcc, so that later on when we get to C++14, yanking it out causes as little churn as possible, with just a 3 letters for 3 letters replacement. I never thought this would be objectionable. If you don't want to do that, that's fine, the churn will happen in the future, but it's no big deal. You'll get to live with shorter make_unique for a few years (my bet is not forever). > > Hope this is constructive > Dave > >
Re: [PATCH 1/2] Add gcc/make-unique.h
On 2022-07-12 7:36 p.m., Pedro Alves wrote: > On 2022-07-12 7:22 p.m., Jonathan Wakely wrote: >> >> >> On Tue, 12 Jul 2022, 17:40 Pedro Alves, > <mailto:pe...@palves.net>> wrote: >> >> On 2022-07-12 4:14 p.m., Jonathan Wakely wrote: >> >> >> So once GCC requires C++14, why would you want to preserve >> >> once-backported symbols in a namespace other than std, when you no >> longer have a reason to? >> >> It will just be another unnecessary thing that newcomers at that >> future time will have >> >> to learn. >> > >> > I also don't see a problem with importing std::make_unique into >> > namespace gcc for local use alongside other things in namespace gcc. I >> > do consider that idiomatic. It says "the make_unique for gcc code is >> > std::make_unique". It means you only need a 'using namespace gcc;' at >> > the top of a source file and you get access to everything in namespace >> > gcc, even if it is something like std::make_unique that was originally >> > defined in a different namespace. >> > >> >> If that's the approach, then GCC should import std::unique_ptr, >> std::move, >> std::foo, std::bar into the gcc namespace too, no? Are you really going >> to propose that? >> >> >> No, I don't follow the logic of "if you do it for one thing you must do it >> for everything". That's a straw man. But I don't really mind how this gets >> done. Your suggestion is fine. >> > > It isn't a strawman, Jon. Maybe there's some miscommunication. The > conversion started (and part of it is > still quoted above), by thinking about what we'd do once we get to C++14, and > my suggestion to optimize > for that. When we get to the point when we require C++14, make_unique is no > longer different from any other > symbol in the std namespace, and there will be no reason to treat it > differently anymore. Like, if someone at > that point proposes to remove the global make_unique or gcc::make_unique, and > replace all references with > std::make_unique, there will be no real ground to object to that, why > wouldn't you want it? This is very > much like when you removed "gnu::unique_ptr" (not going to miss it) a few > months back -- you replaced > it by "std::unique_ptr"; gnu::unique_ptr wasn't kept just because of history. Sorry to reply to myself -- but I'm not sure it is clear what I meant above in the last sentence, so let me try again: 'the "gnu::unique_ptr" wasn't rewritten as an import of std::unique_ptr into the gnu namespace just because of history.'
Re: [PATCH 1/2] Add gcc/make-unique.h
On 2022-07-12 7:22 p.m., Jonathan Wakely wrote: > > > On Tue, 12 Jul 2022, 17:40 Pedro Alves, <mailto:pe...@palves.net>> wrote: > > On 2022-07-12 4:14 p.m., Jonathan Wakely wrote: > > >> So once GCC requires C++14, why would you want to preserve > >> once-backported symbols in a namespace other than std, when you no > longer have a reason to? > >> It will just be another unnecessary thing that newcomers at that > future time will have > >> to learn. > > > > I also don't see a problem with importing std::make_unique into > > namespace gcc for local use alongside other things in namespace gcc. I > > do consider that idiomatic. It says "the make_unique for gcc code is > > std::make_unique". It means you only need a 'using namespace gcc;' at > > the top of a source file and you get access to everything in namespace > > gcc, even if it is something like std::make_unique that was originally > > defined in a different namespace. > > > > If that's the approach, then GCC should import std::unique_ptr, std::move, > std::foo, std::bar into the gcc namespace too, no? Are you really going > to propose that? > > > No, I don't follow the logic of "if you do it for one thing you must do it > for everything". That's a straw man. But I don't really mind how this gets > done. Your suggestion is fine. > It isn't a strawman, Jon. Maybe there's some miscommunication. The conversion started (and part of it is still quoted above), by thinking about what we'd do once we get to C++14, and my suggestion to optimize for that. When we get to the point when we require C++14, make_unique is no longer different from any other symbol in the std namespace, and there will be no reason to treat it differently anymore. Like, if someone at that point proposes to remove the global make_unique or gcc::make_unique, and replace all references with std::make_unique, there will be no real ground to object to that, why wouldn't you want it? This is very much like when you removed "gnu::unique_ptr" (not going to miss it) a few months back -- you replaced it by "std::unique_ptr"; gnu::unique_ptr wasn't kept just because of history. Cheers, Pedro Alves
Re: [PATCH 1/2] Add gcc/make-unique.h
On 2022-07-12 4:14 p.m., Jonathan Wakely wrote: >> So once GCC requires C++14, why would you want to preserve >> once-backported symbols in a namespace other than std, when you no longer >> have a reason to? >> It will just be another unnecessary thing that newcomers at that future time >> will have >> to learn. > > I also don't see a problem with importing std::make_unique into > namespace gcc for local use alongside other things in namespace gcc. I > do consider that idiomatic. It says "the make_unique for gcc code is > std::make_unique". It means you only need a 'using namespace gcc;' at > the top of a source file and you get access to everything in namespace > gcc, even if it is something like std::make_unique that was originally > defined in a different namespace. > If that's the approach, then GCC should import std::unique_ptr, std::move, std::foo, std::bar into the gcc namespace too, no? Are you really going to propose that?
Re: [PATCH 1/2] Add gcc/make-unique.h
On 2022-07-12 2:45 p.m., Jonathan Wakely wrote:
> On Tue, 12 Jul 2022 at 14:24, Pedro Alves wrote:
>>
>> On 2022-07-12 1:25 a.m., David Malcolm via Gcc-patches wrote:
>>
>>> I tried adding it to gcc/system.h, but anything that uses it needs to
>>> have std::unique_ptr declared, which meant forcibly including
>>> from gcc/system.h
>>
>> Did you consider making gcc/system.h include gcc/make-unique.h itself
>> if INCLUDE_MEMORY is defined? Something like:
>>
>> #ifdef INCLUDE_MEMORY
>> # include
>> + #include "make-unique.h"
>> #endif
>>
>> This is because std::make_unique is defined in in C++14. This would
>> mean fewer changes once GCC requires C++14 (or later) and this new header is
>> eliminated.
>
> That's a good idea.
>
>>> (in the root namespace, rather than std::, which saves a bit more typing).
>>
>> It's less typing now, but it will be more churn once GCC requires C++14 (or
>> later), at
>> which point you'll naturally want to get rid of the custom make_unique.
>> More churn
>> since make_unique -> std::make_unique may require re-indentation of
>> arguments, etc.
>> For that reason, I would suggest instead to put the function (and any other
>> straight
>> standard library backport) in a 3-letter namespace already, like,
>> gcc::make_unique
>> or gnu::make_unique. That way, when the time comes that GCC requires C++14,
>> the patch to replace gcc::make_unique won't have to worry about reindenting
>> code,
>> it'll just replace gcc -> std.
>
> Or (when the time comes) don't change gcc->std and do:
>
> namespace gcc {
> using std::make_unique;
> }
It will seem like a pointless indirection then, IMO.
>
> or just leave it in the global namespace as in your current patch, and
> at a later date add a using-declaration to the global namespace:
>
> using std::make_unique;
>
That's not very idiomatic, though. Let me turn this into a reverse question:
If GCC required C++14 today, would you be doing the above, either importing
make_unique
to the global namespace, or into namespace gcc? I think it's safe to say
that, no,
nobody would be doing that. So once GCC requires C++14, why would you want to
preserve
once-backported symbols in a namespace other than std, when you no longer have
a reason to?
It will just be another unnecessary thing that newcomers at that future time
will have
to learn.
Re: [PATCH 1/2] Add gcc/make-unique.h
On 2022-07-12 1:25 a.m., David Malcolm via Gcc-patches wrote: > I tried adding it to gcc/system.h, but anything that uses it needs to > have std::unique_ptr declared, which meant forcibly including > from gcc/system.h Did you consider making gcc/system.h include gcc/make-unique.h itself if INCLUDE_MEMORY is defined? Something like: #ifdef INCLUDE_MEMORY # include + #include "make-unique.h" #endif This is because std::make_unique is defined in in C++14. This would mean fewer changes once GCC requires C++14 (or later) and this new header is eliminated. > (in the root namespace, rather than std::, which saves a bit more typing). It's less typing now, but it will be more churn once GCC requires C++14 (or later), at which point you'll naturally want to get rid of the custom make_unique. More churn since make_unique -> std::make_unique may require re-indentation of arguments, etc. For that reason, I would suggest instead to put the function (and any other straight standard library backport) in a 3-letter namespace already, like, gcc::make_unique or gnu::make_unique. That way, when the time comes that GCC requires C++14, the patch to replace gcc::make_unique won't have to worry about reindenting code, it'll just replace gcc -> std.
Re: [PATCH] c: Implement new -Wenum-int-mismatch warning [PR105131]
On 2022-05-18 00:56, Marek Polacek via Gcc-patches wrote:
> +In C, an enumerated type is compatible with @code{char}, a signed
> +integer type, or an unsigned integer type. However, since the choice
> +of the underlying type of an enumerated type is implementation-defined,
> +such mismatches may cause portability issues. In C++, such mismatches
> +are an error. In C, this warning is enabled by @option{-Wall}.
Should it also be enabled by -Wc++-compat?
Pedro Alves
Re: [PATCH] Remove conditional STATIC_ASSERT.
On 2022-05-05 13:41, Martin Liška wrote: > On 5/5/22 14:29, Richard Biener wrote: >> Can we then use static_assert (...) instead and remove the >> macro? > > Oh yes, we can ;) > >> Do we have C compiled code left (I think we might, >> otherwise we'd not have __cplusplus guards in system.h), >> in which case the #if should change to #ifdef __cplusplus? > > No, there's no such a consumer of the macro. > > What about the updated version of the patch? static_assert without the second/message parameter requires C++17: https://en.cppreference.com/w/cpp/language/static_assert The macro expanded to always have a message argument.
Re: [RFC] Add support for the "retain" attribute utilizing SHF_GNU_RETAIN
On 10/6/20 12:10 PM, Jozef Lawrynowicz wrote: > The changes would also only affect targets > that support the GNU ELF OSABI, which would lead to inconsistent > behavior between non-GNU OS's. Well, a separate __attribute__((retain)) will necessarily only work on GNU ELF targets, so that just shifts the "inconsistent" behavior elsewhere. Pedro Alves
Re: [RFC] Add support for the "retain" attribute utilizing SHF_GNU_RETAIN
On 10/6/20 12:10 PM, Jozef Lawrynowicz wrote: > Should "used" apply SHF_GNU_RETAIN? > === > Another talking point is whether the existing "used" attribute should > apply the SHF_GNU_RETAIN flag to the containing section. > > It seems unlikely that a user applies the "used" attribute to a > declaration, and means for it to be saved from only compiler > optimization, but *not* linker optimization. So perhaps it would be > beneficial for "used" to apply SHF_GNU_RETAIN in some way. > > If "used" did apply SHF_GNU_RETAIN, we would also have to > consider the above options for how to apply SHF_GNU_RETAIN to the > section. Since the "used" attribute has been around for a while > it might not be appropriate for its behavior to be changed to place the > associated declaration in its own, unique section, as in option (2). > To me, if I use attribute((used)), and the linker still garbage collects the symbol, then the toolchain has a bug. Is there any use case that would suggest otherwise? Thanks, Pedro Alves
Re: [PATCH] configure: Re-disable building cross-gdbserver
On 2/11/20 9:01 PM, Maciej W. Rozycki wrote:
> On Tue, 11 Feb 2020, Tom Tromey wrote:
>
>> Maciej> Correct fallout from commit 919adfe84092 ("Move gdbserver to top
>> level")
>> Maciej> and revert to not building `gdbserver' in a cross-configuration,
>> that is
>> Maciej> where host != target, matching the documented behaviour. We have no
>> way
>> Maciej> to support non-native `gdbserver', and native `gdbserver' is usually
>> of
>> Maciej> no use with cross-GDB of the chosen host.
>>
>> Pedro had a different way to do this, that keeps the decision under
>> gdbserver's control:
>>
>> https://sourceware.org/ml/gdb-patches/2020-02/msg00383.html
>
> That's actually quite similar to what I considered first, before I
> changed my mind. Whatever.
Doing it in gdbserver/ has the advantage that it stays under gdbserver's
control, so it doesn't need syncing code with the gcc tree. I know of at
least one off-tree port that uses gdbserver in a host != target scenario,
so I imagine that this condition will evolve over time.
Also, this way, changes in this area don't require running autoconf to
regenerate configure.
I'm not seeing any downside.
>
> However I would expect `exit' not to be what we want in a sourced script
> (I did this differently; see below).
Good point, somehow did not think of that.
It worked in my patch because we source the script in a sub-shell.
But it's clearer/better to not rely on that.
>
> case "${host}" in
> + ${target})
> + gdbserver_host=${host}
> + ;;
> + *)
> + gdbserver_host=NONE
> + ;;
if/else reads more to-the-point to me, so I tweaked it that
way, and merged it in (to binutils-gdb), like below.
I'm sorry for not noticing your earlier patch.
>From f20e3e823d56e54ffe56792ea6a2fe947c2dec0d Mon Sep 17 00:00:00 2001
From: "Maciej W. Rozycki"
Date: Wed, 12 Feb 2020 13:50:30 +
Subject: [PATCH] Disable gdbserver on host != target configurations
Correct fallout from commit 919adfe84092 ("Move gdbserver to top level")
and revert to not building `gdbserver' in a cross-configuration, that is
where host != target, matching the documented behaviour. We have no way
to support non-native `gdbserver', and native `gdbserver' is usually of
no use with cross-GDB of the chosen host.
gdbserver/ChangeLog:
2020-02-12 Maciej W. Rozycki
Pedro Alves
Skip building gdbserver in a cross-configuration.
* configure.srv: Set $gdbserver_host depending on whether $target
is $host. Use $gdbserver_host instead of $host.
---
gdbserver/ChangeLog | 7 +++
gdbserver/configure.srv | 11 +--
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/gdbserver/ChangeLog b/gdbserver/ChangeLog
index 09707067730..709ef23674c 100644
--- a/gdbserver/ChangeLog
+++ b/gdbserver/ChangeLog
@@ -1,3 +1,10 @@
+2020-02-12 Maciej W. Rozycki
+ Pedro Alves
+
+ Skip building gdbserver in a cross-configuration.
+ * configure.srv: Set $gdbserver_host depending on whether $target
+ is $host. Use $gdbserver_host instead of $host.
+
2020-02-11 Simon Marchi
* configure: Re-generate.
diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv
index 2e83cbdc07f..375ac0aeb2a 100644
--- a/gdbserver/configure.srv
+++ b/gdbserver/configure.srv
@@ -33,9 +33,16 @@ ipa_ppc_linux_regobj="powerpc-32l-ipa.o
powerpc-altivec32l-ipa.o powerpc-vsx32l-
# these files over and over again.
srv_linux_obj="linux-low.o nat/linux-osdata.o nat/linux-procfs.o
nat/linux-ptrace.o nat/linux-waitpid.o nat/linux-personality.o
nat/linux-namespaces.o fork-child.o nat/fork-inferior.o"
-# Input is taken from the "${host}" variable.
+# Input is taken from the "${host}" and "${target}" variables.
-case "${host}" in
+# GDBserver can only debug native programs.
+if test "${target}" = "${host}"; then
+gdbserver_host=${host}
+else
+gdbserver_host=
+fi
+
+case "${gdbserver_host}" in
aarch64*-*-linux*) srv_tgtobj="linux-aarch64-low.o"
srv_tgtobj="$srv_tgtobj nat/aarch64-linux-hw-point.o"
srv_tgtobj="$srv_tgtobj linux-aarch32-low.o"
base-commit: 38de8abe21fe17c31888094bd860a84f88cb5749
--
2.14.5
Re: Move -Wmaybe-uninitialized to -Wextra
On 12/16/19 2:45 PM, Martin Jambor wrote: > On Sat, Dec 07 2019, Jeff Law wrote: >> [...] > I'm afraid I that -Wmaybe-uninitialized is getting out of hand. I bet > that some users regularly get these warnings coming from c++ header > "libraries" (like they sometimes come out our vec.h which recently > "broke" bootstrap) which they sometimes even cannot change and they then > conclude that our -Wall is "unusable" and stop paying attention to all > warnings. -Wmaybe-uninitialized that trigger in std::optional (and clones) (PR80635 [1]) are particularly annoying, and there's no sane workaround the user can apply. You'll find quite a number of those just by googling for it: https://www.google.com/search?q=std+optional+"-Wmaybe-uninitialized; We have a few of those in GDB, and because GDB uses -Wall + -Werror, GDB nowadays builds with -Wno-error=maybe-uninitialized so that we see the warnings but the build continues without error. People still occasionally get confused and waste time with those warnings, though. Here, just this week, point 5: https://sourceware.org/ml/gdb-patches/2019-12/msg00706.html FWIW, I've considered completely disabling -Wmaybe-uninitialized in GDB instead of downgrading it from -Werror to a warning with -Wno-error. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80635 -- Pedro Alves
Re: Type representation in CTF and DWARF
On 10/18/19 2:21 PM, Richard Biener wrote: >>> In most cases local types etc are a fairly small contributor to the >>> total volume -- but macros can contribute a lot in some codebases. >> (The >>> Linux kernel's READ_ONCE macro is one I've personally been bitten by >> in >>> the past, with a new local struct in every use. GCC doesn't >> deduplicate >>> any of those so the resulting bloat from tens of thousands of >> instances >>> of this identical structure is quite incredible...) >>> >> >> Sounds like something that would be beneficial to do with DWARF too. > > Otoh those are distinct types according to the C standard and since dwarf is > a source level representation we should preserve this (source locations also > differ). Right. Maybe some partial deduplication would be possible, preserving type distinction. But since CTF doesn't include these, this is moot for now. Thanks, Pedro Alves
Re: Type representation in CTF and DWARF
On 10/17/19 7:59 PM, Nick Alcock wrote: > On 17 Oct 2019, Richard Biener verbalised: > >> On Thu, Oct 17, 2019 at 7:36 PM Nick Alcock wrote: >>> >>> On 11 Oct 2019, Indu Bhagat stated: >>>> Compile with -g -gdwarf-like-ctf and use dwz -o >>>> (using >>>> dwz compiled from the master branch) on the generated binaries: >>>> >>>> (coreutils-0.22) >>>> .debug_info(D1) | .debug_abbrev(D2) | .debug_str(D4) | .ctf >>>> (uncompressed) | ratio (.ctf/(D1+D2+0.5*D4)) >>>> ls 30616 |1136 |21098 | 26240 >>>> | 0.62 >>>> pwd 10734 |788|10433 | 13929 >>>> | 0.83 >>>> groups 10706 |811|10249 | 13378 >>>> | 0.80 >>>> >>>> (emacs-26.3) >>>> .debug_info(D1) | .debug_abbrev(D2) | .debug_str(D4) | .ctf >>>> (uncompressed) | ratio (.ctf/(D1+D2+0.5*D4)) >>>> emacs-26.3.1 674657 |6402 | 273963 | 273910 >>>> | 0.33 >> >> Btw, for a fair comparison you have to remove all DW_TAG_subroutine >> children as well since CTF doesn't represent scopes or local variables >> at all (nor types only used by locals). It seems CTF only represents >> function entry points. > > Good point: I'll have to hack up a DWARF trimmer to do this comparison > properly, I think. (Though CTF does represent global variables, > including file-scope statics.) Wouldn't it be possible to extend the -gdwarf-like-ctf hack to skip emitting those things? > > In most cases local types etc are a fairly small contributor to the > total volume -- but macros can contribute a lot in some codebases. (The > Linux kernel's READ_ONCE macro is one I've personally been bitten by in > the past, with a new local struct in every use. GCC doesn't deduplicate > any of those so the resulting bloat from tens of thousands of instances > of this identical structure is quite incredible...) > Sounds like something that would be beneficial to do with DWARF too. Thanks, Pedro Alves
Re: Type representation in CTF and DWARF
On 10/17/19 6:36 PM, Nick Alcock wrote: > A side note here: the sizes given above are uncompressed sizes, but in > the real world CTF is almost always compressed: the threshold for > compression is in theory customizable but at the moment is hardwired at > 4KiB-uncompressed in the linker. I usually see compression ratios of > roughly 3 or 4 to 1: e.g. I just tried it with a randomly chosen binary, > /usr/lib/libgtk-3.so.0.2404.3, and got these sizes: DWARF can be compressed too, with --compress-debug-sections. Thanks, Pedro Alves
Re: Type representation in CTF and DWARF
On 10/4/19 8:23 PM, Indu Bhagat wrote: > Hello, > > At GNU Tools Cauldron this year, some folks were curious to know more on how > the "type representation" in CTF compares vis-a-vis DWARF. I was one of those, and I brought this up to Jose, after your presentation. Glad to see the follow up! Thanks much for this. In your Cauldron presentation we saw CTF compared to full blown DWARF as justification for CTF, but I was more interested in a comparison between CTF and a DWARF subset containing exactly only what you have available in CTF. Because if DWARF with everything-you-don't-need stripped out is in the same ballpark, then I am puzzled on why add/maintain a new Debug format, with all the duplication of effort that entails going forward. Also, it's my understanding that the current CTF format doesn't yet support C++, Vector registers, etc., maybe other things, so if DWARF was sufficient for your needs, then in the long run it sounds like a better option to me, as then you wouldn't have to extend CTF _and_ DWARF whenever some feature is needed. Maybe it would make sense to work on integrating CTF into the DWARF standard itself, not sure? I was also curious on your plans for adding unwinding support to CTF, while the kernel (the main CTF user, IIUC), already has plans to use its own unwinding format (ORC)? So with all those questions, I came out of the presentation thinking that I could not really justify CTF if I were asked to. (Side note: the Cauldron page is missing slides for your presentation, so I couldn't go and recheck some things mentioned above.) Thanks, Pedro Alves
Re: [PATCH 0/3] add support for POD struct convention (PR 61339)
On 7/12/19 9:24 AM, Jakub Jelinek wrote: > I'd just arrange that when being compiled with clang we compile with > -Wno-mismatched-tags to get rid of their misdesigned warning and not add > such misdesigned warning to GCC, that will just help people spread this > weirdo requirement further. FWIW and FYI, this is what GDB does (gdb/warning.m4). Thanks, Pedro Alves
Re: [PATCH] Teach mklog to reference PRs.
On 8/12/19 8:34 PM, Pedro Alves wrote: > Let me share my 2c -- the format GDB uses doesn't affect most GCC forks I meant most GCC folks. Thanks, Pedro Alves
Re: [PATCH] Teach mklog to reference PRs.
On 8/1/19 4:01 PM, Jakub Jelinek wrote:
> You can easily tweak the script to handle the other way too.
> Looking around, different people have different style, some people don't
> post the date/name/email lines at all, others do, some people post them
> multiple times, once for each ChangeLog, others just once, and for the
> latter case, some people post gcc/, etc. prefixes before the entries
> afterwards, while others don't, and others do it only conditionally
> (e.g. when the number of ChangeLog files is too high or it isn't immediately
> obvious which ones they are for, say one entry for gcc and one for
> gcc/testsuite ChangeLog, or similarly for gcc/cp and gcc/testsuite etc.
> isn't hard to figure out and is just noise.
> So, either we have multiple options for mklog, so that people if they prefer
> some other style don't have to rewrite it all the time, or have one style we
> want to recommend. If the latter, I think it is better to have a style that
> is perhaps automatically parseable by a script, on the other side for
> readers of the mailing list should minimize unnecessary cruft and
> redundancies. For that I think the email line just once, then empty line,
> then PR lines and/or line with short summary as some people use, then
> the dirnames of ChangeLog entries (but no ChangeLog, that is always the
> case) and actual entries sounds best to me.
Let me share my 2c -- the format GDB uses doesn't affect most GCC forks
of course, though uniformity across GNU tools is a good thing to have
in my opinion, to ease sharing tooling and ease moving between projects.
"email just once" assumes that you have a single author or set of authors
for the whole patch. But if you have a patch that includes entries by
different authors, then you have to have multiple ChangeLog "blocks" each
with its author/email line, ending up where you didn't want to to begin
with, anyway, like:
-MM-DD John Doe
gcc/
* ...
-MM-DD Joe Shmoe
gcc/testsuite/
* ...
With the "email line per ChangeLog file entry" style, there's
no exception to the rule, it all just always looks the same, like:
gcc/
-MM-DD John Doe
* ...
gcc/testsuite/
-MM-DD Joe Shmoe
* ...
This isn't your everyday patch, of course, but it does
show up here and there.
IMHO, saving a few characters/bytes doesn't justify the simplicity
of just having a full entry per ChangeLog file.
FWIW, this is the style used by GDB, as documented at the end
of section 9, at
<https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog>.
"In your patch email you should also specify which changelog is being modified.
Before the line containing the date and your name/email, add a line with the
path to the changelog. If there are multiple components being updated with
multiple changelog edits, split these into sections, one for each changelog:
gdb/ChangeLog:
2013-12-12 John Doe
PR gdb/
* breakpoint.c (handle_some_event): Remove reference to foo. Call
bar.
* configure.ac (build_warnings): Do not use -Wformat-nonliteral
with -Wno-format.
* configure: Regenerate.
* NEWS: Mention awesome feature.
gdb/testsuite/ChangeLog:
2013-12-12 John Doe
PR gdb/9999
* Makefile: Test changes for awesome feature.
"
Pedro Alves
Re: Move -Wmaybe-uninitialized to -Wextra
On 02/05/2019 05:07 PM, Jeff Law wrote: > FWIW, I've been doing Fedora rawhide builds with gcc-9 since early Jan. > Not everything builds with -Wall -Werror, but lots of packages do. > I've only seen one maybe-uninit warning cause problems -- it was > spurious and for a memory object. I didn't dig into it at all. Do you have insight into how many Fedora packages explicitly disable maybe-uninit? I really don't know the answer to that, but I'd at least keep that in mind as a possible reason for the warning seemingly not causing trouble often. GDB disables it (or rather, removes it from -Werror), and I wouldn't be surprised if other C++ programs do the same, when they start using std::optional more. Since boost::optional also triggers the same warnings, set may be even larger than C++11-or-later programs. I don't know how to search the Fedora codebase, but a github search for "-Wno-maybe-uninitialized" shows > 100k hits: https://github.com/search?q=-Wno-maybe-uninitialized=Code and a search for -Wno-error=maybe-uninitialized shows >10k: https://github.com/search?q=-Wno-error%3Dmaybe-uninitialized=Code That's a much higher hit rate than say -Wno-array-bounds or -Wno-format-overflow, or -Wno-stringop-overflow, which were some searches I tried. > > In contrast things like the missing NUL termination warnings, buffer > overflow warnings, NULL strings to *printf* warnings, etc all caught > numerous (dozens) of real bugs. I mention it because it's one of the > ways I know packages are building with -Wall -Werror :-) Thanks, Pedro Alves
Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536
On 12/11/2018 02:25 PM, Ian Lance Taylor wrote: > On Tue, Dec 11, 2018 at 3:05 AM Pedro Alves wrote: >> Ian earlier mentioned that we've wanted to avoid malloc because some >> programs call the demangler from a signal handler, but it seems like >> we already do, these functions already aren't safe to use from >> signal handlers as is. Where does the "we can't use malloc" idea >> come from? Is there some entry point that avoids >> the malloc/realloc/free calls? > > cplus_demangle_v3_callback and cplus_demangle_print_callback. Ah, gotcha. Thanks! Interesting. Pedro Alves
Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536
On 12/11/2018 06:58 AM, Jakub Jelinek wrote: > On Mon, Dec 10, 2018 at 05:33:19PM -0700, Jeff Law wrote: >>>> where di.num_comps is just strlen (mangled) * 2. Without any analysis >>>> whatsoever, bumping the "recursion" limit will just mean we can process 1.5 >>>> times long names. Either we need more precise analysis on what we are >>>> looking for (how big arrays we'll need) or it needs to be an independent >>>> limit and certainly should allow say 10KB symbols too if they are >>>> reasonable. >>> >>> If the problem is alloca, we could avoid using alloca if the size >>> passes a threshold. Perhaps even use a better data structure than a >>> preallocated array based on a guess about the number of components... >> Actually I would strongly suggest avoiding alloca completely. This >> isn't particularly performance sensitive code and alloca can be abused >> in all kinds of interesting ways. > > We can't use malloc, I noticed that the comment on top of __cxa_demangle says: "If OUTPUT_BUFFER is not long enough, it is expanded using realloc." and __cxa_demangle calls 'free'. And d_demangle, seemingly the workhorse for __cxa_demangle says: /* Entry point for the demangler. If MANGLED is a g++ v3 ABI mangled name, return a buffer allocated with malloc holding the demangled name. OPTIONS is the usual libiberty demangler options. On success, this sets *PALC to the allocated size of the returned buffer. On failure, this sets *PALC to 0 for a bad name, or 1 for a memory allocation failure, and returns NULL. */ cplus_demangle, the entry point that gdb uses, also relies on malloc. Ian earlier mentioned that we've wanted to avoid malloc because some programs call the demangler from a signal handler, but it seems like we already do, these functions already aren't safe to use from signal handlers as is. Where does the "we can't use malloc" idea come from? Is there some entry point that avoids the malloc/realloc/free calls? Not advocating for adding to the number of malloc calls willy-nilly BTW. I'd prefer to reduce the number of mallocs/rellocs calls within the demangler and also within the bfd_demangle wrapper, for performance reasons, for example by making it possible to reuse a growing buffer across demangling invocations. > therefore on some targets alloca (or VLAs) are the only > option, and for small sizes even if mmap is available using it is too > costly. > > Though, I like Jason's suggestion of just adding a maxinum of the number > of components and number of substitutions and failing if we need more. > > Jakub Thanks, Pedro Alves
Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536
On 12/11/2018 12:33 AM, Jeff Law wrote: > Actually I would strongly suggest avoiding alloca completely. This > isn't particularly performance sensitive code On the contrary, the demangler is very performance-sensitive code for GDB. See <https://sourceware.org/ml/binutils/2018-11/msg00257.html> and Tromey's response. > and alloca can be abused > in all kinds of interesting ways. Thanks, Pedro Alves
Re: RFC: libiberty PATCH to disable demangling of ancient mangling schemes
Adding gdb-patches, since demangling affects gdb. Ref: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00407.html On 12/07/2018 10:40 AM, Jakub Jelinek wrote: > On Fri, Dec 07, 2018 at 10:27:17AM +, Nick Clifton wrote: >>>> Looks good to me. Independently, do you see a reason not to disable the >>>> old demangler entirely? >>> >>> Like so. Does anyone object to this? These mangling schemes haven't >>> been relevant in decades. >> >> I am not really familiar with this old scheme, so please excuse my ignorance >> in asking these questions: >> >> * How likely is it that there are old toolchain in use out there that >> still >> use the v2 mangling ? Ie I guess that I am asking "which generation(s) >> of gcc used v2 mangling ?" > > GCC 3.0 and up used the new (Itanium C++ ABI) mangling, 2.95 and older used > the old > mangling (2.96-RH used the new mangling I believe). > So you need compiler older than 17.5 years to have the old mangling. > Such a compiler didn't support most of the contemporarily used platforms > though at all (e.g. x86-64, powerpc64le, aarch64, I believe not even > powerpc64-linux). > Yeah. I guess the question would be whether it is reasonable to expect that people will still need to debug (with gdb, c++filt, etc.) any such old binary, and that they will need to do it with with modern tools, as opposed to sticking with older binutils, and how often would that be needed. I would say that it's very, very unlikely, and not worth it of the maintenance burden. Last I heard of 2.95-produced binaries I think was for some ancient gcc-2.95-based cross compiler that was still being minimally maintained, because it was needed to build some legacy stuff. That was maybe over 8 years ago, and it was off trunk. It's probably dead by now. And if isn't dead, whoever maintains the compiler off trunk certainly can also maintain old-ish binutils & gdb off trunk. Thanks, Pedro Alves
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]
On 12/04/2018 04:56 PM, Nick Clifton wrote: > Hi Pedro, > >> The issue pointed out by >> >> https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02592.html >> >> is still present in this version. > > Doh! Yes I meant to fix that one too, but forgot. > >> Also, noticed a typo here: >> >>> +/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as >> >> Typo: "RECURE" > > Oops - thanks. > > OK, revised (v5) patch attached. Is this version acceptable to all ? > This is fine with me. Thanks, Pedro Alves
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v4]
On 12/04/2018 01:59 PM, Nick Clifton wrote: > OK then, here is a fourth revision of the patch. The issue pointed out by https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02592.html is still present in this version. Also, noticed a typo here: > +/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as Typo: "RECURE" > + the maximum depth of recursion allowed. It should be enough for any > + real-world mangled name. */ > +#define DEMANGLE_RECURSION_LIMIT 1024 > + Thanks, Pedro Alves
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]
On 11/30/2018 05:41 PM, Nick Clifton wrote:
> @@ -4693,10 +4694,21 @@
> demangle_nested_args (struct work_stuff *work, const char **mangled,
>string *declp)
> {
> + static unsigned long recursion_level = 0;
>string* saved_previous_argument;
>int result;
>int saved_nrepeats;
>
> + if ((work->options & DMGL_RECURSE_LIMIT)
> + && work->recursion_level > DEMANGLE_RECURSION_LIMIT)
> +{
> + /* FIXME: There ought to be a way to report that the recursion limit
> + has been reached. */
> + return 0;
> +}
> +
> + recursion_level ++;
> +
There's still a static recursion level counter here?
Thanks,
Pedro Alves
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler
On 11/30/2018 08:26 AM, Nick Clifton wrote:
> Hi Pedro, Hi Tom,
>
>> Pedro> E.g., in GDB, loading big binaries, demangling is very high
>> Pedro> in profiles, and so we've kicked around the desire to parallelize
>> Pedro> it
>
> I did consider this, but I encountered two problems:
>
> 1. I wanted users of the demangling functions to be able to change
> the recursion limit. So for example in environments with a very
> limited amount of stack space the limit could be reduced. This
> is one of the purposes of the cplus_demangle_set_recursion_limit()
> function.
>
> Since a new d_info structure is created every time a string is demangled
> the only way to implement cplus_demangle_set_recursion_limit would
> be to have it set the value that is used to initialize the field in
> the structure, which is basically back to where we are now.
That's a lesser evil. With the proposed cplus_demangle_set_recursion_limit
interface, you essentially end up with an interface that makes all threads in
the process end up with the same limit. There's precedent for global options
in the current_demangling_style global, set by the cplus_demangle_set_style
function. That's one I recalled, there may be others. I'd prefer interfaces
that pass down the options as arguments, or a pointer to an options struct, but
that's not the main issue.
The main issue is the current recursion level counter.
That's the static variable that I had pointed at:
d_function_type (struct d_info *di)
{
[...]
+ static unsigned long recursion_level = 0;
[...]
+ recursion_level ++;
[...]
+ recursion_level --;
return ret;
}
Even if we go with a recursion limit per process, this recursion
level count must be moved to d_info for thread-safety without
synchronization.
>
> 2. I wanted to be able to access the recursion limit from code
> in cplus-dem.c, which does not use the d_info structure.
That should just mean a bit of plumbing is in order to pass down
either a d_info structure or something like it?
> This was
> the other purpose of the cplus_demangler_set_recursion_limit()
> function - it allows the limit to be read without changing it.
>
> As an alternative I did consider adding an extra parameter to the
> cplus_demangle(), cplus_demangle_opname() and related functions. But
> this would make them non-backwards compatible and I did not want to
> do that.
I'd say that ideally, we'd aim at the interface we want, and then go
from there. If changing interfaces is a problem, we can always add
a new entry point and keep the old as backward compatibility.
But is it (a problem)? Do we require ABI compatibility here?
But again, the main issue I'm seeing is the recursion level counter,
not the global limit.
> I would imagine that changing the recursion limit would be a very
> rare event, possibly only ever done once in an executable's runtime,
> so I doubt that there will ever be any real-life thread safety
> problems associated with it. But I do understand that this is just
> an assumption, not a guarantee.
See above.
Thanks,
Pedro Alves
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler
Hi Nick,
On 11/29/2018 03:01 PM, Nick Clifton wrote:
> static struct demangle_component *
> d_function_type (struct d_info *di)
> {
> - struct demangle_component *ret;
> + static unsigned long recursion_level = 0;
Did you consider making this be a part of struct d_info instead?
IIRC, d_info is like a "this" pointer, passed around pretty
much everywhere.
I think going in the direction of making the demangler harder to use
in an efficient thread-safe manner is undesirable, even if the feature
is optional. E.g., in GDB, loading big binaries, demangling is very high
in profiles, and so we've kicked around the desire to parallelize
it (e.g., by parallelizing the reading/interning of DSO files, instead of
reading all of them sequentially). Having to synchronize access to the
demangler would be quite unfortunate. If possible, it'd be great
to avoid making work toward that direction harder. (Keeping in mind that
if this recursion detection feature is useful for binutils, then it should
also be useful for GDB.)
Thanks,
Pedro Alves
Re: Compilation error in simple-object-elf.c
On 07/19/2018 02:06 PM, Eli Zaretskii wrote: >> From: Richard Biener >> Date: Thu, 19 Jul 2018 10:46:01 +0200 >> Cc: DJ Delorie , GCC Patches , >> gdb-patc...@sourceware.org >> >>> *err = ENOTSUP; >>>^~~ >>> ./simple-object-elf.c:1284:14: note: each undeclared identifier is >>> reported only once for each function it appears in >>> >>> Suggested fix: >> >> Works for me, thus OK. I'm going to check it in to make 8.2. > > Thanks. > > Joel/Pedro, is this okay for GDB's copy of libiberty, master and > branch? Yes. Thanks, Pedro Alves
Re: [PATCH][RFC] Make iterating over hash-map elide copying/destructing
On 07/11/2018 12:24 PM, Trevor Saunders wrote: > However if we went that route we should prevent use of the > assignment operator by declaring one explicitly and making it private but > then not implementing it, so it at least fails to link and with some > macros you can actually tell the compiler in c++11 its deleted and may > not be used. The macro already exists --- DISABLE_COPY_AND_ASSIGN in include/ansidecl.h. Thanks, Pedro Alves
Re: [PATCH (for gdb)] enum-flags.h: Add trailing semicolon to example in comment
[adding gdb-patches] On 06/05/2018 06:56 PM, David Malcolm wrote: > On Tue, 2018-06-05 at 17:13 +0100, Pedro Alves wrote: >> On 06/05/2018 03:49 PM, David Malcolm wrote: >>> On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote: >>>> You may want to look at gdb's enum-flags.h which I think already >>>> implements what your doing here. >>> >>> Aha! Thanks! >>> >>> Browsing the git web UI, that gdb header was introduced by Pedro >>> Alves >>> in: >>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit >>> diff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9 >>> and the current state is here: >>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f >>> =gdb/common/enum-flags.h;hb=HEAD >>> >>> I'll try this out with GCC; hopefully it works with C++98. >> >> It should -- it was written/added while GDB still required C++98. > > Thanks. > >> Note I have a WIP series here: >> >> https://github.com/palves/gdb/commits/palves/cxx11-enum-flags >> >> that fixes a few things, and adds a bunch of unit tests. In >> the process, it uses C++11 constructs (hence the branch's name), >> but I think that can be reverted to still work with C++98. >> >> I had seen some noises recently about GCC maybe considering >> requiring C++11. Is that reasonable to expect in this cycle? >> (GDB requires GCC 4.8 or later, FWIW.) > > I'm expecting that GCC 9 will still require C++98. OK. > >> Getting that branch into master had fallen lower on my TODO, >> but if there's interest, I can try to finish it off soon, so we >> can share a better baseline. (I posted it once, but found >> some issues which I fixed since but never managed to repost.) > > Interestingly, it looks like gdb is using Google Test for selftests; > gcc is using a hand-rolled API that is similar, but has numerous > differences (e.g. explicit calling of test functions, rather than > implicit test discovery). So that's another area of drift between > the projects. Nope, the unit tests framework is hand rolled too. See gdb/selftest.h/c. It can be invoked with gdb's "maint selftest" command. You can see the list of tests with "maint info selftests", and you can pass a filter to "maint selftest" too. > >>> >>> Presumably it would be good to share this header between GCC and >>> GDB; >>> CCing Pedro; Pedro: hi! Does this sound sane? >>> (for reference, the GCC patch we're discussing here is: >>> https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html ) >> >> Sure! > > Alternatively, if GCC needs to stay at C++98 and GDB moves on to C++11, > then we can at least take advantage of this tested and FSF-assigned > C++98 code (better than writing it from scratch). Agreed, but I'll try to see about making the fixes in the branch C++98 compatible. > > I ran into one issue with the header, e.g. with: > > /* Dump flags type. */ > DEF_ENUM_FLAGS_TYPE(enum dump_flag, dump_flags_t); > > This doesn't work: > TDF_SLIM | flags > but this does: > flags | TDF_SLIM > where TDF_SLIM is one of the values of "enum dump_flag". ISTR that that's fixed in the branch. > BTW, I spotted this trivial issue in a comment in enum-flags.h whilst > trying it out for gcc: > > > > The DEF_ENUM_FLAGS_TYPE macro should be used with a trailing > semicolon, but the example in the comment lacks one. > > * enum-flags.h: Add trailing semicolon to example in comment. Thanks. I've merged it. >From 115f7325b5b76450b9a1d16848a2a54f54e49747 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Tue, 5 Jun 2018 18:22:25 +0100 Subject: [PATCH] Fix typo in common/enum-flags.h example The DEF_ENUM_FLAGS_TYPE macro should be used with a trailing semicolon, but the example in the comment lacks one. gdb/ChangeLog: 2018-06-05 David Malcolm * common/enum-flags.h: Add trailing semicolon to example in comment. --- gdb/ChangeLog | 5 + gdb/common/enum-flags.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 85c62dbd86c..54205a6ea85 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2018-06-05 David Malcolm + + * common/enum-flags.h: Add trailing semicolon to example in + comment. + 2018-06-05 Tom Tromey PR cli/12326: diff --git a/gdb/common/enum-flags.h b/gdb/common/enum-flags.h index 65732c11a46..82568a5a3d9 100644 --- a/gdb/common/enum-flags.h +++ b/gdb/common/enum-flags.h @@ -32,7 +32,7 @@ flag_val3 = 1 << 3, flag_val4 = 1 << 4, }; -DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags) +DEF_ENUM_FLAGS_TYPE(enum some_flag, some_flags); some_flags f = flag_val1 | flag_val2; f |= flag_val3; -- 2.14.3 Thanks, Pedro Alves
Re: Sharing gdb's enum-flags.h with gcc? (was Re: [PATCH 01/10] Convert dump and optgroup flags to enums)
On 06/05/2018 03:49 PM, David Malcolm wrote: > On Tue, 2018-06-05 at 04:40 -0400, Trevor Saunders wrote: >> You may want to look at gdb's enum-flags.h which I think already >> implements what your doing here. > > Aha! Thanks! > > Browsing the git web UI, that gdb header was introduced by Pedro Alves > in: > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=8d297bbf604c8318ffc72d5a7b3db654409c5ed9 > and the current state is here: > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/common/enum-flags.h;hb=HEAD > > I'll try this out with GCC; hopefully it works with C++98. It should -- it was written/added while GDB still required C++98. Note I have a WIP series here: https://github.com/palves/gdb/commits/palves/cxx11-enum-flags that fixes a few things, and adds a bunch of unit tests. In the process, it uses C++11 constructs (hence the branch's name), but I think that can be reverted to still work with C++98. I had seen some noises recently about GCC maybe considering requiring C++11. Is that reasonable to expect in this cycle? (GDB requires GCC 4.8 or later, FWIW.) Getting that branch into master had fallen lower on my TODO, but if there's interest, I can try to finish it off soon, so we can share a better baseline. (I posted it once, but found some issues which I fixed since but never managed to repost.) > > Presumably it would be good to share this header between GCC and GDB; > CCing Pedro; Pedro: hi! Does this sound sane? > (for reference, the GCC patch we're discussing here is: > https://gcc.gnu.org/ml/gcc-patches/2018-05/msg01685.html ) Sure! Thanks, Pedro Alves
Re: ATTRIBUTE_NONSTRING
On 04/27/2018 02:41 AM, Alan Modra wrote: > This patch adds ATTRIBUTE_NONSTRING, which will be used to curb > -Wstringop-truncation warnings in binutils. OK to apply? > > * ansidecl.h (ATTRIBUTE_NONSTRING): Define. +1, FWIW. Thanks, Pedro Alves
Re: [PATCH] Add ax_pthread.m4 for use in binutils-gdb
On 04/17/2018 11:10 PM, Joshua Watt wrote: > On Tue, 2018-04-17 at 22:50 +0100, Pedro Alves wrote: >> On 04/17/2018 06:24 PM, Joshua Watt wrote: >>> Ping? I'd really like to get this in binutils, which apparently >>> requires getting it here first. >> >> I think it would help if you mentioned what this is and >> what is the intended use case. > > Ah, that would probably be helpful! Yes, this was discussed on the > binutils mailing list, see: > https://sourceware.org/ml/binutils/2018-02/msg00260.html > > In short summary: the gold linker doesn't currently build for mingw, > but only because it is attempting to link against libpthread > incorrectly on that platform. Instead of bringing in more specialized > logic to account for that, I opted to include the autotools > ax_pthread.m4 macro (this patch) that automatically handles discovering > pthreads on a wide variety of platforms and compilers, including mingw. > > binutils slaves its config/ directory to GCC, so the patch is required > to be committed here first, and then it will be ported over there. Thanks, that helps indeed. I agree that the ax_pthread.m4 approach makes sense. Better to use a field-tested macro than reinvent the wheel. We're using other files from the autoconf-archive archive already, for similar reasons (e.g., config/ax_check_define.m4, and gdb/ax_cxx_compile_stdcxx.m4). Since GCC won't be using it (yet at least, but it's conceivable it could make use of it in future), there should be no harm in installing it even if GCC is in stage 4, IMO. I don't have the authority to approve it, though. Thanks, Pedro Alves
Re: [PATCH] Add ax_pthread.m4 for use in binutils-gdb
On 04/17/2018 06:24 PM, Joshua Watt wrote: > Ping? I'd really like to get this in binutils, which apparently > requires getting it here first. I think it would help if you mentioned what this is and what is the intended use case. Was this discussed on the binutils or gdb lists? Thanks, Pedro Alves
Re: [documentation patch] add detail to const and pure attributes
On 04/02/2018 11:34 PM, Martin Sebor wrote:
> On 04/02/2018 12:09 AM, Sandra Loosemore wrote:
>> On 03/27/2018 03:21 PM, Pedro Alves wrote:
>>> On 03/27/2018 09:19 PM, Martin Sebor wrote:
>>>> On 03/27/2018 01:38 PM, Pedro Alves wrote:
>>>>> On 03/27/2018 07:18 PM, Martin Sebor wrote:
>>>>>> +Because a @code{pure} function can have no side-effects it does not
>>>>>
>>>>> FWIW, I'd suggest rephrasing as:
>>>>>
>>>>> Because a @code{pure} function cannot have side effects
>>>>>
>>>>> because "can have no side-effects" can be read as
>>>>> "is allowed to have no side effects", which gave me pause
>>>>> when I read it the first time, and is the opposite of
>>>>> what you mean.
>>>>
>>>> That is what I meant: that const and pure functions are not allowed
>>>> to have any side-effects. If they did, they could be unexpectedly
>>>> eliminated (i.e., the behavior is undefined when such a function
>>>> does have a side-effect).
>>>
>>> I know, but that's not what I read the first time (and found it
>>> odd so I had to re-read). You can either assume that I'm the
>>> only one that will misunderstand it on first read, or you can
>>> swap a couple words and be sure no one will misunderstand it.
>>>
>>> Up to you.
>>
>> I'm chiming in a little late here, but I agree with Pedro that "can have
>> no side-effects" is confusing. I'd say "cannot have side effects" or
>> "must have no side effects" instead.
>
> There's nothing confusing about it. It's an established phrase
> with millions of uses and only one meaning. According to Google
> Books Ngram Viewer it's also more pervasive than either of
> the two suggested alternatives:
>
> http://goo.gl/FgXgwi
Sorry, but no. The different phrases have slightly different
meanings. The fact that one is used more often than than the other
does not imply that they have the same meaning, any more than this:
http://goo.gl/U64uDZ
shows that people nowadays say "red" more often when then
mean "blue".
Compare:
#1. The red pill can have no side effects. (If you're lucky. It's
not guaranteed. Buyer beware.)
#2. The blue pill cannot have side effects. (That's a guarantee.)
Those are different statements. #1 implies possibility, while #2
leaves no margin for error.
When you say that "a pure function can have no side effects",
that can be reasonably read as
a pure function may have no side effects if it chooses to,
i.e., it's not required to have side effects.
But, a pure function _must_ have no side effects. If a function has
side effects, then it no longer is a pure function, by definition.
That's what reads confusingly.
Your url actually proves the point. Follow the url at the bottom
of that page:
https://www.google.pt/search?q=%22can+have+no+effect%22=bks=lang_en_rd=cr=0=NUPDWqqoPInxUqP8jPgN
And that leads to uses like:
"In many cases, a treatment can have no effect or can have the effect"
"the new grant can have no effect whatever, unless it have the effect"
etc., etc.
>
>> Also note that non-hyphenated "side effects" seems to be preferred usage
>> as a noun phrase (at least it's the only form listed by m-w.com).
>
> I'm all for using the preferred form. I've made the change here
> and submitted a separate patch to remove the hyphen from the rest
> of the occurrences.
>
> Attached is a changed patch that uses "cannot have side effects"
> instead of "can have no side effects."
Thank you. LGTM, FWIW.
Pedro Alves
Re: [documentation patch] add detail to const and pure attributes
On 03/27/2018 09:19 PM, Martin Sebor wrote:
> On 03/27/2018 01:38 PM, Pedro Alves wrote:
>> On 03/27/2018 07:18 PM, Martin Sebor wrote:
>>> +Because a @code{pure} function can have no side-effects it does not
>>
>> FWIW, I'd suggest rephrasing as:
>>
>> Because a @code{pure} function cannot have side effects
>>
>> because "can have no side-effects" can be read as
>> "is allowed to have no side effects", which gave me pause
>> when I read it the first time, and is the opposite of
>> what you mean.
>
> That is what I meant: that const and pure functions are not allowed
> to have any side-effects. If they did, they could be unexpectedly
> eliminated (i.e., the behavior is undefined when such a function
> does have a side-effect).
I know, but that's not what I read the first time (and found it
odd so I had to re-read). You can either assume that I'm the
only one that will misunderstand it on first read, or you can
swap a couple words and be sure no one will misunderstand it.
Up to you.
>
> I don't have a strong preference for one phrasing over the other
> but they both say the same thing. One is just ever so slightly
> more emphatic.
>
Thanks,
Pedro Alves
Re: [documentation patch] add detail to const and pure attributes
On 03/27/2018 07:18 PM, Martin Sebor wrote:
> +Because a @code{pure} function can have no side-effects it does not
FWIW, I'd suggest rephrasing as:
Because a @code{pure} function cannot have side effects
because "can have no side-effects" can be read as
"is allowed to have no side effects", which gave me pause
when I read it the first time, and is the opposite of
what you mean.
Thanks,
Pedro Alves
Re: [PATCH] adjust warning_n() to take uhwi (PR 84207)
On 02/14/2018 09:47 PM, Manuel López-Ibáñez wrote:
> On 14 Feb 2018 8:16 pm, "Pedro Alves" <pal...@redhat.com
> <mailto:pal...@redhat.com>> wrote:
>
> Instead of a class that has to have a constructor for every type
> you want to pass as plural selector to the _n functions, which
> increases coupling, I'd suggest using a conversion function, and
> overload that. I.e., something like, in the core diagnostics code:
>
> static inline unsigned HOST_WIDE_INT
> as_plural_form (unsigned HOST_WIDE_INT val)
> {
> return val;
> }
>
> /* In some tree-diagnostics header. */
>
> static inline unsigned HOST_WIDE_INT
> as_plural_form (tree t)
> {
> // extract & return a HWI
> }
>
> /* In some whatever-other-type-diagnostics header. */
>
> static inline unsigned HOST_WIDE_INT
> as_plural_form (whatever_other_type v)
> {
> // like above
> }
>
> and then you call error_n and other similar functions like this:
>
> error_n (loc, u, "%u thing", "%u things", u);
> error_n (loc, as_plural_form (u), "%u thing", "%u things", u);
> error_n (loc, as_plural_form (t), "%E thing", "%E things", t);
> error_n (loc, as_plural_form (i), "%wu thing", "%wu things", i);
>
> This is similar in spirit to std::to_string, etc.
>
>
> If that's desired, why not simply have GCC::to_uhwi() ? It would likely be
> useful in other contexts.
Because of types that (e.g. wide_int specializations) that can store
values larger than what fit in uhwi. GCC::to_uhwi's semantics for that
aren't clear -- could saturate, could unsigned wrap, could
throw/abort/assert, could be undefined.
as_plural_form has clear semantics -- it'd return a value that does
the right thing for ngettext's N parameter. I.e., it'd do
the "n % 100 + 100" operation as a wide_int still, and
before converting/cast the result to uhwi.
Thanks,
Pedro Alves
Re: [PATCH] adjust warning_n() to take uhwi (PR 84207)
On 02/13/2018 10:37 PM, Martin Sebor wrote:
> On 02/13/2018 01:59 PM, Manuel López-Ibáñez wrote:
>>
> Here's a sketch of what I tried to do:
>
> struct IntegerConverter
> {
> union {
> tree t;
> unsigned HOST_WIDE_INT hwi;
> // buffer for offset_int, wide_int, etc.
> } value;
>
> IntegerConverter (tree t)
> {
> value.t = t;
> }
>
> IntegerConverter (unsigned HOST_WIDE_INT x)
> {
> value.x = x;
> }
>
> ...
> };
>
> void error_n (int, const IntegerConverter &, const char*, ...);
> ...
>
> With that, the call
>
> error_n (loc, t, "%E thing", "%E things", t);
>
> works when t is a tree, and the call to the same function
>
> error_n (loc, i, "%wu thing", "%wu things", i);
>
> also works when i is a HOST_WIDE_INT. I chose this approach
> to avoid introducing additional overloads of the error_n()
> functions.
Instead of a class that has to have a constructor for every type
you want to pass as plural selector to the _n functions, which
increases coupling, I'd suggest using a conversion function, and
overload that. I.e., something like, in the core diagnostics code:
static inline unsigned HOST_WIDE_INT
as_plural_form (unsigned HOST_WIDE_INT val)
{
return val;
}
/* In some tree-diagnostics header. */
static inline unsigned HOST_WIDE_INT
as_plural_form (tree t)
{
// extract & return a HWI
}
/* In some whatever-other-type-diagnostics header. */
static inline unsigned HOST_WIDE_INT
as_plural_form (whatever_other_type v)
{
// like above
}
and then you call error_n and other similar functions like this:
error_n (loc, u, "%u thing", "%u things", u);
error_n (loc, as_plural_form (u), "%u thing", "%u things", u);
error_n (loc, as_plural_form (t), "%E thing", "%E things", t);
error_n (loc, as_plural_form (i), "%wu thing", "%wu things", i);
This is similar in spirit to std::to_string, etc.
BTW, the "plural_form" naming above comes from ngettext's documentation
of the N parameter:
char * ngettext (const char *msgid1, const char *msgid2, unsigned long int n);
"The parameter n is used to determine the plural form."
Pedro Alves
Re: [C++ PATCH] Plural forms for count != eltscnt structured binding diagnostics
On 11/30/2017 01:10 PM, Jakub Jelinek wrote:
> On Thu, Nov 30, 2017 at 01:33:48PM +0100, Jakub Jelinek wrote:
>> On Thu, Nov 30, 2017 at 01:01:58PM +0100, Jakub Jelinek wrote:
>>> I just followed:
>>> https://www.gnu.org/savannah-checkouts/gnu/gettext/manual/html_node/Plural-forms.html
>>>
>>> "About larger integer types, such as ‘uintmax_t’ or ‘unsigned long long’:
>>> they can be
>>> handled by reducing the value to a range that fits in an ‘unsigned long’.
>>> Simply
>>> casting the value to ‘unsigned long’ would not do the right thing, since it
>>> would
>>> treat ULONG_MAX + 1 like zero, ULONG_MAX + 2 like singular, and the like.
>>> Here you
>>> can exploit the fact that all mentioned plural form formulas eventually
>>> become periodic,
>>> with a period that is a divisor of 100 (or 1000 or 100). So, when you
>>> reduce a large
>>> value to another one in the range [100, 199] that ends in the same
>>> 6 decimal
>>> digits, you can assume that it will lead to the same plural form selection.
>>> This code
>>> does this:
>>>
>>> #include
>>> uintmax_t nbytes = ...;
>>> printf (ngettext ("The file has %"PRIuMAX" byte.",
>>> "The file has %"PRIuMAX" bytes.",
>>> (nbytes > ULONG_MAX
>>>? (nbytes % 100) + 100
>>>: nbytes)),
>>> nbytes);"
>>>
>>> I can surely add a comment about that.
How about wrapping it in a function to make it self-describing?
Something around:
/* Comment/url here. */
unsigned long
plural_form_for (unsigned HOST_WIDE_INT val)
{
return (val > ULONG_MAX
? (val % 100) + 100
: val);
}
and then:
inform_n (loc, plural_form_for (eltscnt),
"while %qT decomposes into %wu element",
"while %qT decomposes into %wu elements",
type, eltscnt);
Pedro Alves
Re: [PING] Plugin support on Windows/MinGW
On 11/23/2017 12:06 PM, Boris Kolpackov wrote: > JonY <10wa...@gmail.com> writes: > >> Libtool shouldn't matter since it is not used to build those, [...] > > We don't know which build system the plugin author will use to build > the plugin. We can, however, reasonably expect that it will be able > to produce a shared library with the platform-standard extensions > (.dll, .dylib, etc). > > >> I'll commit in a few days if there are no more inputs. > > Great, thanks! I would just like to say that I think this is a fantastic patch that will help open GCC to many more use cases. The fact that plugins don't work on Windows has been a sore spot, IMO. I for one am very happy that this make gdb's libcc1 plugin a viable option for Windows. Puts us one step closer to the long term plan of making GDB always use GCC for C/C++ expression parsing/evaluation. Yay. :-) Thanks, Pedro Alves
Re: [RFA][PATCH] patch 5/n Cleaning up evrp
On 11/17/2017 04:49 AM, Jeff Law wrote: > + /* We do not allow copying this object or initializing one from another. > */ > + evrp_dom_walker (const evrp_dom_walker &); > + evrp_dom_walker& operator= (const evrp_dom_walker &); > + Note you can use include/ansidecl.h's DISABLE_COPY_AND_ASSIGN for this [1], and then you don't need the comment, as it's self-documenting. [1] - https://marc.info/?l=gcc-patches=150549025810729=2 (gcc.gnu.org seems to be unreachable for me today.) Thanks, Pedro Alves
Re: [patch] backwards threader cleanups
On 11/15/2017 07:34 AM, Aldy Hernandez wrote: > > > On 11/14/2017 02:38 PM, David Malcolm wrote: >> On Tue, 2017-11-14 at 14:08 -0500, Aldy Hernandez wrote: > >>https://gcc.gnu.org/codingconventions.html#Class_Form >> says that: >> >> "When defining a class, first [...] >> declare all public member functions, >> [...] >> then declare all non-public member functions, and >> then declare all non-public member variables." > > Wow, I did not expect that order. Fixed. ... >> (Is this a self-assign from this->speed_p? should the "speed_p" param >> be renamed, e.g. to "speed_p_") > > Yes. Fixed. The convention also says: "When structs and/or classes have member functions, prefer to name data members with a leading m_". So in this case, the preference would be to rename this->speed_p to m_speed_p instead. Thanks, Pedro Alves
Re: [006/nnn] poly_int: tree constants
On 10/27/2017 12:29 AM, Martin Sebor wrote: > > IMO, a good rule of thumb to follow in class design is to have > every class with any user-defined ctor either define a default > ctor that puts the object into a determinate state, or make > the default ctor inaccessible (or deleted in new C++ versions). > If there is a use case for leaving newly constructed objects > of a class in an uninitialized state that's an exception to > the rule that can be accommodated by providing a special API > (or in C++ 11, a defaulted ctor). Yet another rule of thumb is to make classes that model built-in types behave as close to the built-in types as possible, making it easier to migrate between the custom types and the built-in types (and vice versa), to follow expectations, and to avoid pessimization around e.g., otherwise useless forcing initialization of such types in containers/arrays when you're going to immediately fill in the container/array with real values. BTW, there's a proposal for adding a wide_int class to C++20: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0539r1.html and I noticed: ~~~ 26.??.2.?? wide_integer constructors [numeric.wide_integer.cons] constexpr wide_integer() noexcept = default; Effects: A Constructs an object with undefined value. ~~~ Thanks, Pedro Alves
Re: [RFA][PATCH] Provide a class interface into substitute_and_fold.
On 10/25/2017 06:20 PM, Jeff Law wrote: > My conclusion on the virtual dtor issue is that it's not strictly needed > right now. > > IIUC the issue is you could do something like > > base *foo = new derived (); > [ ... ] > delete foo; > > If the base's destructor is not virtual and foo is a base * pointing to > a derived object then the deletion invokes undefined behavior. > > In theory we shouldn't be doing such things :-) In fact, if there's a > way to prevent this with some magic on the base class I'd love to know > about it. There is: make the base class destructor protected. Thanks, Pedro Alves
Re: [09/nn] Add a fixed_size_mode_pod class
On 10/27/2017 09:35 AM, Richard Biener wrote: > On Thu, Oct 26, 2017 at 9:43 PM, Jakub Jelinek <ja...@redhat.com> wrote: >> On Thu, Oct 26, 2017 at 02:43:55PM +0200, Richard Biener wrote: >>> Can you figure what oldest GCC release supports the C++11/14 POD handling >>> that would be required? >> >> I think it is too early for that, we aren't LLVM or Rust that don't really >> care about what build requirements they impose on users. > > That's true, which is why I asked. For me requiring sth newer than GCC 4.8 > would be a blocker given that's the system compiler on our latest server > (and "stable" OSS) product. > > I guess it depends on the amount of pain we have going forward with C++ > use in GCC. Given that gdb already requires C++11 people building > GCC are likely already experiencing the "issue". Right, GDB's baseline is GCC 4.8 too. When GDB was deciding whether to start requiring full C++11 (about a year ago), we looked at the latest stable release of all the "big" distros to see whether: #1 - the system compiler was new enough (gcc >= 4.8), or failing that, #2 - whether there's an easy to install package providing a new-enough compiler. and it turns out that that was true for all. Meanwhile another year has passed and there have been no complaints. Thanks, Pedro Alves
Re: [006/nnn] poly_int: tree constants
On 10/26/2017 07:54 PM, Martin Sebor wrote:
> (...) in the general case, it is preferable to
> declare each variable at the point where its initial value is known
> (or can be computed) and initialize it on its declaration.
With that I fully agree, except it's not always possible or
natural. The issue at hand usually turns up with
conditional initialization, like:
void foo ()
{
int t;
if (something)
t = 1;
else if (something_else)
t = 2;
if (t == 1)
bar ();
}
That's a simple example of course, but more complicated
conditionals aren't so easy to grok and spot the bug.
In the case above, I'd much prefer if the compiler tells me
I missed initializing 't' than initializing it to 0 "just
in case".
Thanks,
Pedro Alves
Re: [006/nnn] poly_int: tree constants
On 10/26/2017 05:37 PM, Martin Sebor wrote:
> I agree that the latter use case is more common in GCC, but I don't
> see it as a good thing. GCC was written in C and most code still
> uses now outdated C practices such as declaring variables at the top
> of a (often long) function, and usually without initializing them.
> It's been established that it's far better to declare variables with
> the smallest scope, and to initialize them on declaration. Compilers
> are smart enough these days to eliminate redundant initialization or
> assignments.
I don't agree that that's established. FWIW, I'm on the
"prefer the -Wuninitialized" warnings camp. I've been looking
forward to all the VRP and threader improvements hoping that that
warning (and -Wmaybe-uninitialized...) will improve along.
> My comment is not motivated by convenience. What I'm concerned
> about is that defining a default ctor to be a no-op defeats the
> zero-initialization semantics most users expect of T().
This sounds like it's a problem because GCC is written in C++98.
You can get the semantics you want in C++11 by defining
the constructor with "= default;" :
struct T
{
T(int); // some other constructor forcing me to
// add a default constructor.
T() = default; // give me default construction using
// default initialization.
int i;
};
And now 'T t;' leaves T::i default initialized, i.e.,
uninitialized, while T() value-initializes T::i, i.e.,
initializes it to zero.
So if that's a concern, maybe you could use "= default"
conditionally depending on #if __cplusplus >= C++11, so that
you'd get it for stages after stage1.
Or just start requiring C++11 already. :-)
Thanks,
Pedro Alves
Re: [PATCH] Add INCLUDE_UNIQUE_PTR and use it (PR bootstrap/82610)
On 10/23/2017 07:15 PM, David Malcolm wrote: > OK for trunk? FAOD, FWIW, LGTM. Thanks, Pedro Alves
Re: [PATCH] Include from system.h (PR bootstrap/82610)
On 10/23/2017 04:50 PM, David Malcolm wrote: > FWIW, this one isn't from #pragma poison, it's from: > #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__) > > (I messed up the --in-reply-to when posting the patch, but Gerald noted > the issue was due to: > /usr/include/c++/v1/typeinfo:199:2: error: no member named > 'fancy_abort' in namespace 'std::__1'; did you mean simply > 'fancy_abort'? > _VSTD::abort(); > ^~~ > /usr/include/c++/v1/__config:390:15: note: expanded from macro '_VSTD' > #define _VSTD std::_LIBCPP_NAMESPACE > ^ > /scratch/tmp/gerald/gcc-HEAD/gcc/system.h:725:13: note: 'fancy_abort' > declared here > extern void fancy_abort (const char *, int, const char *) > ^ > IMO the best fix would be to rename that "#define abort" to "#define gcc_abort" and then call gcc_abort instead in the few places that currently call abort. IME, the introduction of a new naked call to abort() isn't something that easily passes review. abort calls always stand out and give reviewers pause (or they should!). FWIW, GDB also doesn't want such naked abort() calls, I don't recall people-sneaking-in-abort-()-calls ever being a problem over there. Thanks, Pedro Alves
Re: [PATCH] Include from system.h (PR bootstrap/82610)
On 10/23/2017 04:17 PM, Jonathan Wakely wrote: > On 23/10/17 17:07 +0200, Michael Matz wrote: >> Hi, >> >> On Mon, 23 Oct 2017, Richard Biener wrote: >> >>> I guess so. But we have to make gdb happy as well. It really depends how >>> much each TU grows with the extra (unneeded) include grows in C++11 and >>> C++04 mode. >> >> The c++ headers unconditionally included from system.h, with: >> >> % echo '#include <$name>' | g++-7 -E -x c++ - | wc -l >> new: 3564 >> cstring: 533 >> utility: 3623 >> memory: 28066 > > That's using the -std=gnu++4 default for g++-7, and for that mode > the header *is* needed, to get the definition of std::unique_ptr. > > For C++98 (when it isn't needed) that header is much smaller: > > tmp$ echo '#include ' | g++ -E -x c++ - | wc -l > 28101 > tmp$ echo '#include ' | g++ -E -x c++ - -std=gnu++98 | wc -l > 4267 > > (Because it doesn't contain std::unique_ptr and std::shared_ptr before > C++11). > >> compile time: >> % echo -e '#include <$name>\nint i;' | time g++-7 -c -x c++ - >> new: 0:00.06elapsed, 17060maxresident, 0major+3709minor >> cstring: 0:00.03elapsed, 13524maxresident, 0major+3075minor >> utility: 0:00.05elapsed, 16952maxresident, 0major+3776minor >> memory: 0:00.25elapsed, 40356maxresident, 0major+9764minor >> >> Hence, is not cheap at all, including it unconditionally from >> system.h when it isn't actually used by many things doesn't seem a good >> idea. >> I think the real question is whether it makes a difference in a full build. There won't be many translation units that don't include some other headers. (though of course I won't be surprised if it does make a difference.) If it's a real issue, you could fix this like how the other similar cases were handled by system.h, by adding this in system.h: #ifdef __cplusplus #ifdef INCLUDE_UNIQUE_PTR # include "unique-ptr.h" #endif #endif instead of unconditionally including there, and then translation units that want unique-ptr.h would do "#define INCLUDE_UNIQUE_PTR" instead of #include "unique-ptr.h", like done for a few other C++ headers. (I maintain that IMO this is kind of self-inflicted GCC pain due to the fact that "#pragma poison" poisons too much. If #pragma poison's behavior were adjusted (or a new variant/mode created) to ignore references to the poisoned symbol names in system headers (or something like that), then you wouldn't need this manual management of header dependencies in gcc/system.h and the corresponding '#define INCLUDE_FOO' contortions. There's nothing that you can reasonably do with a reference to a poisoned symbol in a system header, other than avoid having the system header have the '#pragma poison' in effect when its included, which leads to contortions like system.h's. Note that the poisoned names are _still used anyway_. So can we come up with a GCC change that would avoid having to worry about manually doing this? It'd likely help other projects too.) Thanks, Pedro Alves
Re: [PATCH] Include from system.h (PR bootstrap/82610)
On 10/23/2017 02:51 PM, Richard Biener wrote: > On Mon, Oct 23, 2017 at 2:58 PM, David Malcolm <dmalc...@redhat.com> wrote: >> OK for trunk? > > Not entirely happy as unique-ptr.h doesn't use but well. > Actually it does. It's needed in C++11 mode, because that's where std::unique_ptr is defined: #if __cplusplus >= 201103 /* In C++11 mode, all we need is import the standard std::unique_ptr. */ template using unique_ptr = std::unique_ptr; > Ok to unbreak bootstrap. Thanks, Pedro Alves
Re: [PATCH] Make -gcolumn-info the default
On 10/23/2017 02:46 PM, Jason Merrill wrote: > On Mon, Oct 23, 2017 at 3:33 AM, Jakub Jelinek <ja...@redhat.com> wrote: >> Hi! >> >> When -gcolumn-info was added back in February, it was too late in the >> release cycle to make it the default, but I think now is the good time >> to do it for GCC8. >> >> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Makes sense to me. +1 from me, FWIW. Thanks, Pedro Alves
Re: [patch] implement generic debug() functions for wide_int's
On 10/18/2017 06:08 PM, Aldy Hernandez wrote: > Also, do we have a blessed way of specifying overloaded functions in > ChangeLog's? I couldn't find anything in our GCC coding guidelines or > in the GNU coding guidelines. For lack of direction, I'm doing the > following: > > * wide-int.cc (debug) [const wide_int &]: New. > (debug) [const wide_int *]: New. > (debug) [const widest_int &]: New. > (debug) [const widest_int *]: New. > > It seems appropriate, as that is the GNU way of changelogs for a > conditional change to a function ???. Doesn't seem that appropriate to me. Square brackets are used for conditional compilation (#ifdef etc.), but overloads are not that. I'd suggest looking in libstdc++'s ChangeLog for precedents. It's where I looked at when I had the same question for GDB, FWIW. E.g., a very recent libstdc++ commit from Jon had: * include/bits/stl_map.h (map::insert(value_type&&)) (map::insert(const_iterator, value_type&&)): Add overload for rvalues. I.e., simply use the full prototype as function name, since it's really what it is in C++. Thanks, Pedro Alves
Re: [PATCH] Implement unique_xmalloc_ptr<T[]> and add more selftests
On 10/14/2017 12:35 AM, David Malcolm wrote:
> As far as I can tell from your mail, the one issue that blocks that
> is the lack of gdb::unique_xmalloc_ptr<T[]>.
>
> So here's an patch on top of the previous one which adds the
> xmalloc_deleter<T[]> (taken from gdb, but changing "xfree" to
> "free", and adding support for pre-C++11 dialects), along with
> selftests for unique_ptr<T[]>, unique_xmalloc_ptr and
> unique_xmalloc_ptr<T[]>.
Thanks much!
> As before, successfully bootstrapped & regrtested on x86_64-pc-linux-gnu,
> using gcc 4.8 for the initial bootstrap (hence testing both gnu++03
> then gnu++14 in the selftests, for stage 1 and stages 2 and 3
> respectively).
>
Excellent.
> Hand-tested with "make selftest-valgrind" with both gnu++03 and
> gnu++14.
>
> Also tested stage1 on powerpc-ibm-aix7.1.3.0 ("gcc111" in the
> compile farm; gcc 4.8 i.e. gnu++03)
>
> Is this OK?
Looks great to me.
> +/* Verify that gnu::unique_malloc_ptr works. */
Typo: malloc -> xmalloc. Appears in other comments too.
Thanks,
Pedro Alves
Re: [PATCH] Add gnu::unique_ptr
On 10/13/2017 10:26 AM, Richard Biener wrote: > On Fri, Oct 13, 2017 at 2:40 AM, David Malcolm <dmalc...@redhat.com> wrote: >> From: Trevor Saunders <tbsaunde+...@tbsaunde.org> >> >> I had a go at updating Trevor's unique_ptr patch from July >> ( https://gcc.gnu.org/ml/gcc-patches/2017-07/msg02084.html ) >> >> One of the sticking points was what to call the namespace; there was >> wariness about using "gtl" as the name. >> >> Richi proposed (https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00155.html): >>> If it should be short use g::. We can also use gnu:: I guess and I >>> agree gnutools:: is a little long (similar to libiberty::). Maybe >>> gt:: as a short-hand for gnutools. >> >> Pedro noted (https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00157.html): >>> Exactly 3 letters has the nice property of making s/gtl::foo/std::foo/ >>> super trivial down the road; you don't have to care about reindenting >>> stuff >> >> Hence this version of the patch uses "gnu::" - 3 letters, one of the >> ones Richi proposed, and *not* a match for ".tl" (e.g. "gtl"); >> (FWIW personally "gnu::" is my favorite, followed by "gcc::"). >> >> The include/unique-ptr.h in this patch is identical to that posted >> by Trevor in July, with the following changes (by me): >> - renaming of "gtl" to "gnu" >> - renaming of DEFINE_GDB_UNIQUE_PTR to DEFINE_GNU_UNIQUE_PTR >> - renaming of xfree_deleter to xmalloc_deleter, and making it >> use "free" rather than "xfree" (which doesn't exist) >> >> I also went and added a gcc/unique-ptr-tests.cc file containing >> selftests (my thinking here is that although std::unique_ptr ought >> to already be well-tested, we need to ensure that the fallback >> implementation is sane when building with C++ prior to C++11). >> >> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu, >> using gcc 4.8 for the initial bootstrap (hence testing both gnu+03 >> and then gnu++14 in the selftests, for stage 1 and stages 2 and 3 >> respectively). >> >> I also manually tested selftests with both gcc 4.8 and trunk on >> the same hardware (again, to exercise both the with/without C++11 >> behavior). >> >> Tested with "make selftest-valgrind" (no new issues). >> >> OK for trunk? > > Ok if the gdb folks approve The new tests looks good to me. [ The class too, obviously. :-) ] (and will change to this version). GDB used this emulation/shim in master for a period, but it no longer does, because GDB switched to requiring C++11 (gcc >= 4.8) instead meanwhile... I.e., GDB uses standard std::unique_ptr nowadays. GDB still uses gdb::unique_xmalloc_ptr though. Might make sense to switch to using gnu::unique_xmalloc_ptr instead. GDB does have an xfree function that we call instead of free throughout (that's where the reference David fixed comes from). We can most probably just switch to free too nowadays. Also, gdb nowadays also has a gdb::unique_xmalloc_ptr<T[]> specialization that this patch is missing (because the specialization was added after switching to C++11): [pushed] Allow gdb::unique_xmalloc_ptr<T[]> https://sourceware.org/ml/gdb-patches/2017-08/msg00212.html So we'd need that before switching. I don't recall off hand whether it's easy to make that work with the C++03 version. Thanks, Pedro Alves
Re: [PATCH] Add a warning for invalid function casts
On 10/11/2017 03:57 AM, Martin Sebor wrote: > > > [X] This can be function that takes an argument of an incomplete > type, such as: > > struct Incomplete; > typedef void Uncallable (struct Incomplete); > > Any function can safely be converted to Uncallable* without > the risk of being called with the wrong arguments. The only > way to use an Uncallable* is to explicitly convert it to > a pointer to a function that can be called. OOC, did you consider trying to get something like that added to C proper, to some standard header? I don't imagine that it'd be much objectionable, and having a standard type may help tooling give better diagnostics and suggestions. Thanks, Pedro Alves
Re: [PATCH] Add a warning for invalid function casts
On 10/11/2017 03:57 AM, Martin Sebor wrote: > > > Incidentally, void(*)(void) in C++ is a poor choice for this > use case also because of the language's default function > arguments. It's an easy mistake for a C++ programmer to make > to assume that given, say: > > void foo (const char *s = "..."); > > or for any other function that provides default values for all > its arguments, the function may be callable via void(*)(void): > > typedef void F (void); > > void (pf)(void) = (F*)foo; I'd think it'd be much more common to write instead: typedef void F (void); F *pf = (F*)foo; I.e., use the typedef on both sides of the assignment. > > by having the (default) function argument value(s) magically > substituted at the call site of: > >pf (); > > Bu since that's not the case it would be helpful for the new > warning to detect this mistake. By encouraging the use of > > typedef void F (...); > > as the type of a pointer there is little chance of making such > a mistake. (and then) I don't think I understand this rationale. If users follow the advice, they'll end up with: void foo (const char *s = "..."); typedef void F (...); F *pf = (F *)foo; pf (); which still compiles silently and calls the foo function incorrectly. Thanks, Pedro Alves
Re: [PATCH] Add a warning for invalid function casts
On 10/11/2017 06:58 PM, Martin Sebor wrote:
> On 10/11/2017 11:26 AM, Joseph Myers wrote:
>> On Tue, 10 Oct 2017, Martin Sebor wrote:
>>
>>> The ideal solution for 1) would be a function pointer that can
>>> never be used to call a function (i.e., the void* equivalent
>>> for functions).[X]
>>
>> I don't think that's relevant. The normal idiom for this in modern C
>> code, if not just using void *, is void (*) (void), and since the warning
>> is supposed to be avoiding excessive false positives and detecting the
>> cases that are likely to be used for ABI-incompatible calls, the warning
>> should allow void (*) (void) there.
>
> I think we'll just have to agree to disagree. I'm not convinced
> that using void(*)(void) for this is idiomatic or pervasive enough
> to drive design decisions. Bernd mentioned just libgo and libffi
> as the code bases that do, and both you and I have noted that any
> pointer type works equally well for this purpose. The problem
> with almost any type, including void(*) (void), is that they can
> be misused to call the incompatible function. Since the sole
> purpose of this new warning is to help find those misuses,
> excluding void(*)(void) from the checking is directly at odds
> with its goal.
Switching type-erased function pointers from "void(*)(void)"
to "void(*)()" (in C) substantially increases the risk of code
calling the type-erased pointer without casting it back by accident:
extern void foo (int, int);
typedef void (type_erased_func) (void);
type_erased_func *func = (type_erased_func *) foo;
int main ()
{
func (1, 2); // error: too many arguments
}
vs:
extern void foo (int, int);
typedef void (type_erased_func) (); // note: void dropped
type_erased_func *func = (type_erased_func *) foo;
int main ()
{
func (1, 2); // whoops, now silently compiles
}
I think it'd be good if this were weighed in as well. If 'void ()'
is picked as the special type, then maybe the above could
be at least addressed in the documentation, and/or
diagnostics/notes.
Thanks,
Pedro Alves
Re: correct attribute ifunc C++ type safety (PR 82301)
On 09/25/2017 02:03 AM, Martin Sebor wrote:
> +a @option{-Wincompatible-pointer-types} warning for mismatches. To suppress
> +a warning for the necessary cast from a pointer to the implementation member
> +function to the type of the corresponding non-member function use the
> +@option{-Wno-pmf-conversions} option. For example:
FWIW, it seems odd to me to tell users they need to suppress warnings, when
the compiler surely could provide better/safer means to avoid needing
to use the reinterpret_cast hammer. See below.
> +
> +@smallexample
> +class S
> +@{
> +private:
> + int debug_impl (int);
> + int optimized_impl (int);
> +
> + typedef int Func (S*, int);
> +
> + static Func* resolver ();
> +public:
> +
> + int interface (int);
> +@};
> +
> +int S::debug_impl (int) @{ /* @r{@dots{}} */ @}
> +int S::optimized_impl (int) @{ /* @r{@dots{}} */ @}
> +
> +S::Func* S::resolver ()
> +@{
> + int (S::*pimpl) (int)
> += getenv ("DEBUG") ? ::debug_impl : ::optimized_impl;
> +
> + // Cast triggers -Wno-pmf-conversions.
> + return reinterpret_cast<Func*>(pimpl);
> +@}
> +
If I were writing code like this, I'd write a reinterpret_cast-like
function specifically for pointer-to-member-function to free-function
casting, and only suppress the warning there instead of disabling
the warning for the whole translation unit. Something like:
#include
template struct pmf_as_func;
template
struct pmf_as_func
{
typedef Ret (func_type) (S *, Args...);
typedef S struct_type;
};
template
typename pmf_as_func::func_type *
pmf_as_func_cast (Pmf pmf)
{
static_assert (!std::is_polymorphic::struct_type>::value,
"");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpmf-conversions"
return reinterpret_cast::func_type *> (pmf);
#pragma GCC diagnostic pop
}
and then write:
return pmf_as_func_cast (pimpl);
instead of:
return reinterpret_cast<Func*>(pimpl);
The point being of course to make it harder to misuse the casts.
But that may be a bit too much for the manual.
It also wouldn't work as is with C++03 (because variatic templates).
Which leads me to think that if GCC guarantees this cast works, then
it'd be nice to have GCC provide it (like a __pmf_as_func_cast function)
as builtin. Then it'd work on C++03 as well, and the compiler of course
can precisely validate whether the cast is valid. (It's quite possible
that there's a better check than is_polymorphic as I've written above.)
Just a passing thought.
Thanks,
Pedro Alves
Re: [demangler] Fix nested generic lambda
On 09/15/2017 05:15 PM, Pedro Alves wrote: > On 09/15/2017 01:04 PM, Nathan Sidwell wrote: >> >> >> Pedro, would you like me to port to gdb's libiberty, or will you do a >> merge in the near future? > > I wasn't planning to, but I'm doing it now. > Now done: https://sourceware.org/ml/gdb-patches/2017-09/msg00421.html > Thanks much for the fix! Thanks, Pedro Alves
Re: [demangler] Fix nested generic lambda
On 09/15/2017 01:04 PM, Nathan Sidwell wrote: > > > Pedro, would you like me to port to gdb's libiberty, or will you do a > merge in the near future? I wasn't planning to, but I'm doing it now. Thanks much for the fix! -- Pedro Alves
[pushed] Fix compile time error when using ansidecl.h with an old version of GCC.
Hi guys, I was looking at merging libiberty from gcc to binutils-gdb, and noticed this one patch that is in binutils-gdb and not in gcc, since last July. I think the patch is borderline obvious (it's arguable whether to define OVERRIDE/FINAL for C), but in interest of re-syncing the trees, I'm pushing the patch to gcc as is. Thanks, Pedro Alves >From 47ba729a29c6fa2283835d95d2ab5695d8c5d732 Mon Sep 17 00:00:00 2001 From: Nick Clifton <ni...@redhat.com> Date: Mon, 31 Jul 2017 15:08:32 +0100 Subject: [PATCH] Fix compile time error when using ansidecl.h with an old version of GCC. Binutils PR 21850 * ansidecl.h (OVERRIDE): Protect check of __cplusplus value with #idef __cplusplus. --- include/ChangeLog | 6 ++ include/ansidecl.h | 30 ++ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/include/ChangeLog b/include/ChangeLog index c7ce259..4703588 100644 --- a/include/ChangeLog +++ b/include/ChangeLog @@ -8,6 +8,12 @@ * simple-object.h (simple_object_copy_lto_debug_sections): New function. +2017-07-31 Nick Clifton <ni...@redhat.com> + + Binutils PR 21850 + * ansidecl.h (OVERRIDE): Protect check of __cplusplus value with + #idef __cplusplus. + 2017-07-02 Jan Kratochvil <jan.kratoch...@redhat.com> * dwarf2.def (DW_IDX_compile_unit, DW_IDX_type_unit, DW_IDX_die_offset) diff --git a/include/ansidecl.h b/include/ansidecl.h index f6e1761..ab3b895 100644 --- a/include/ansidecl.h +++ b/include/ansidecl.h @@ -334,22 +334,28 @@ So instead we use the macro below and test it against specific values. */ For gcc, use "-std=c++11" to enable C++11 support; gcc 6 onwards enables this by default (actually GNU++14). */ -#if __cplusplus >= 201103 -/* C++11 claims to be available: use it. final/override were only - implemented in 4.7, though. */ -# if GCC_VERSION < 4007 +#if defined __cplusplus +# if __cplusplus >= 201103 + /* C++11 claims to be available: use it. Final/override were only + implemented in 4.7, though. */ +# if GCC_VERSION < 4007 +# define OVERRIDE +# define FINAL +# else +# define OVERRIDE override +# define FINAL final +# endif +# elif GCC_VERSION >= 4007 + /* G++ 4.7 supports __final in C++98. */ # define OVERRIDE -# define FINAL +# define FINAL __final # else -# define OVERRIDE override -# define FINAL final + /* No C++11 support; leave the macros empty. */ +# define OVERRIDE +# define FINAL # endif -#elif GCC_VERSION >= 4007 -/* G++ 4.7 supports __final in C++98. */ -# define OVERRIDE -# define FINAL __final #else -/* No C++11 support; leave the macros empty: */ + /* No C++11 support; leave the macros empty. */ # define OVERRIDE # define FINAL #endif -- 2.5.5
Re: [PATCH] Add -std=c++2a
On 09/15/2017 01:53 PM, Jakub Jelinek wrote:
> On Mon, Aug 07, 2017 at 02:17:17PM +0100, Pedro Alves wrote:
>> I happened to skim this patch and notice a couple issues.
>> See below.
>
> Note I've just posted updated patch based on this to gcc-patches.
Thanks ( FWIW :-) ).
>>> @@ -497,7 +499,10 @@ cpp_init_builtins (cpp_reader *pfile, int hosted)
>>>
>>>if (CPP_OPTION (pfile, cplusplus))
>>> {
>>> - if (CPP_OPTION (pfile, lang) == CLK_CXX1Z
>>> + if (CPP_OPTION (pfile, lang) == CLK_CXX2A
>>> + || CPP_OPTION (pfile, lang) == CLK_GNUCXX2A)
>>> + _cpp_define_builtin (pfile, "__cplusplus 201707L");
>>
>> I think you wanted 202007L here.
>
> The documentation states some unspecified value strictly greater than
> 201703L. In the patch I've posted it is 201709L, because that is this
> month, 202007L would be just a wild guess. People aren't supposed to
> rely on a particular value until C++2z is finalized, so just use
> (__cplusplus > 201703L) for features beyond C++17.
>
I see, I had assumed 202007L was the intention because that's
what was in the changeLog entry, and because "2020":
Add support for C++2a.
* include/cpplib.h (c_lang): Add CXX2A and GNUCXX2A.
* init.c (lang_defaults): Add rows for CXX2A and GNUCXX2A.
(cpp_init_builtins): Set __cplusplus to 202007L for C++2x.
^^
Thanks,
Pedro Alves
Re: [C++ PATCH] Renames/adjustments of 1z to 17
On 09/14/2017 09:26 PM, Jakub Jelinek wrote:
> +@item c++17
> +@itemx c++1z
> +The 2017 ISO C++ standard plus amendments.
> +The name @samp{c++1z} is deprecated.
> +
> +@item gnu++17
> +@itemx gnu++1z
> +GNU dialect of @option{-std=c++17}.
> +The name @samp{gnu++17} is deprecated.
> @end table
I think you meant to say that gnu++1z is deprecated, not gnu++17.
Thanks,
Pedro Alves
Re: [Ping^2][PATCH, DWARF] Add DW_CFA_AARCH64_negate_ra_state to dwarf2.def/h and dwarfnames.c
On 09/05/2017 09:05 PM, Jiong Wang wrote: > 2017-08-22 9:18 GMT+01:00 Jiong Wang <jiong.w...@foss.arm.com>: >> On 10/08/17 17:39, Jiong Wang wrote: >>> >>> Hi, >>> >>> A new vendor CFA DW_CFA_AARCH64_negate_ra_state was introduced for >>> ARMv8.3-A >>> return address signing, it is multiplexing DW_CFA_GNU_window_save in CFA >>> vendor >>> extension space. >>> >>> This patch adds necessary code to make it available to external, the GDB >>> patch (https://sourceware.org/ml/gdb-patches/2017-08/msg00215.html) is >>> intended >>> to use it. >>> >>> A new DW_CFA_DUP for it is added in dwarf2.def. The use of DW_CFA_DUP >>> is to >>> avoid duplicated case value issue when included in libiberty/dwarfnames. >>> >>> Native x86 builds OK to make sure no macro expanding errors. >>> >>> OK for trunk? >>> >>> 2017-08-10 Jiong Wang <jiong.w...@arm.com> >>> >>> include/ >>> * dwarf2.def (DW_CFA_AARCH64_negate_ra_state): New DW_CFA_DUP. >>> * dwarf2.h (DW_CFA_DUP): New define. >>> >>> libiberty/ >>> * dwarfnames.c (DW_CFA_DUP): New define. >>> I'd like to add a +1 vote for this patch. I was confused more than once in the iterations of the pending gdb patch that were adding references to "DW_CFA_GNU_window_save" in Aarch64 code that has absolutely nothing to do with SPARC register windows, and asked Jiong whether we could add an Aarch64-specific name for the constant. Thanks, Pedro Alves
Re: [PATCH 0/2] add unique_ptr class
On 09/05/2017 05:52 PM, Manuel López-Ibáñez wrote: > On 05/08/17 20:05, Pedro Alves wrote: >> That'd be an "obvious" choice, and I'm not terribly against it, >> though I wonder whether it'd be taking over a name that has a wider >> scope than intended? I.e., GNU is a larger set of projects than the >> GNU toolchain. For example, there's Gnulib, which already compiles >> as libgnu.a / -lgnu, which might be confusing. GCC doesn't currently >> use Gnulib, but GDB does, and, there was work going on a while ago to >> make GCC use gnulib as well. > > Unfortunately, that work was never committed, although there are parts > that are ready to be committed and the rest of the conversion could be > done incrementally: > > https://gcc.gnu.org/wiki/replacelibibertywithgnulib Yeah, ISTR it was close, though there were a couple things that needed addressing still. The wiki seems to miss a pointer to following iterations/review of that patch (mailing list archives don't cross month boundaries...). You can find it starting here: https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01208.html I think this was the latest version posted: https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01554.html Thanks, Pedro Alves
Re: [PATCH 0/2] add unique_ptr class
On 09/04/2017 11:31 AM, Richard Biener wrote: > On Fri, Aug 11, 2017 at 10:43 PM, Jonathan Wakely <jwak...@redhat.com> wrote: >> On 05/08/17 20:05 +0100, Pedro Alves wrote: >>> >>> On 08/04/2017 07:52 PM, Jonathan Wakely wrote: >>>> >>>> On 31/07/17 19:46 -0400, tbsaunde+...@tbsaunde.org wrote: >>>>> >>>>> I've been saying I'd do this for a long time, but I'm finally getting to >>>>> importing the C++98 compatable unique_ptr class Pedro wrote for gdb a >>>>> while >>>>> back. >>> >>> >>> Thanks a lot for doing this! >>> >>>> I believe the gtl namespace also comes from Pedro, but GNU template >>>> library seems as reasonable as any other name I can come up with. >>> >>> >>> Yes, I had suggested it here: >>> >>> https://sourceware.org/ml/gdb-patches/2017-02/msg00197.html >>> >>>> >>>> If it's inspired by "STL" then can we call it something else? >>>> >>>> std::unique_ptr is not part of the STL, because the STL is a library >>>> of containers and algorithms from the 1990s. std::unique_ptr is >>>> something much newer that doesn't originate in the STL. >>>> >>>> STL != C++ Standard Library >>> >>> >>> That I knew, but gtl was not really a reference to the >>> C++ Standard Library, so I don't see the problem. It was supposed to >>> be the name of a library which would contain other C++ utilities >>> that would be shared by different GNU toolchain components. >>> Some of those bits would be inspired by, or be straight backports of >>> more-recent standards, but it'd be more than that. >>> >>> An option would be to keep "gtl" as acronym, but expand it >>> to "GNU Toolchain Library" instead. >> >> >> OK, that raises my hackles less. What bothered me was an apparent >> analogy to "STL" when that isn't even the right name for the library >> that provides the original unique_ptr. >> >> And "template library" assumes we'd never add non-templates to it, >> which is unlikely (you already mentioned offset_type, which isn't a >> template). >> >> It seems odd to make up a completely new acronym for this though, >> rather than naming it after something that exists already in the >> toolchain codebases. >> >> >>> For example, gdb has been growing C++ utilities under gdb/common/ >>> that might be handy for gcc and other projects too, for example: >>> >>> - enum_flags (type-safe enum bit flags) >>> - function_view (non-owning reference to callables) >>> - offset_type (type safe / distinct integer types to represent offsets >>>into anything addressable) >>> - optional (C++11 backport of libstdc++'s std::optional) >>> - refcounted_object.h (intrusively-refcounted types) >>> - scoped_restore (RAII save/restore of globals) >>> - an allocator that default-constructs using default-initialization >>> instead of value-initialization. >>> >>> and more. >>> >>> GCC OTOH has code that might be handy for GDB as well, like for >>> example the open addressing hash tables (hash_map/hash_table/hash_set >>> etc.). >>> >>> Maybe Gold could make use of some of this code too. There >>> are some bits in Gold that might be useful for (at least) GDB >>> too. For example, its Stringpool class. >>> >>> I think there's a lot of scope for sharing more code between the >>> different components of the GNU toolchain, even beyond general >>> random utilities and data structures, and I'd love to see us >>> move more in that direction. >>> >>>> If we want a namespace for GNU utilities, what's wrong with "gnu"? >>> >>> >>> That'd be an "obvious" choice, and I'm not terribly against it, >>> though I wonder whether it'd be taking over a name that has a wider >>> scope than intended? I.e., GNU is a larger set of projects than the >>> GNU toolchain. For example, there's Gnulib, which already compiles >>> as libgnu.a / -lgnu, which might be confusing. GCC doesn't currently >>> use Gnulib, but GDB does, and, there was work going on a while ago to >>> make GCC use gnulib as well. >> >> >> Good point. "gnutools" might be more accurate, but people might object >> to adding 10 extra
Re: [libcc1] Improve detection of triplet on compiler names
On 08/23/2017 05:17 AM, Sergio Durigan Junior wrote:
> Hi there,
>
> This is a series of two patches, one for GDB and one for GCC, which aims
> to improve the detection and handling of triplets present on compiler
> names. The motivation for this series was mostly the fact that GDB's
> "compile" command is broken on Debian unstable, as can be seen here:
>
> <https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=851146>
>
> The reason for the failure is the fact that Debian compiles GCC using
> the --program-{prefix,suffix} options from configure in order to name
> the compiler using the full triplet (i.e., Debian's GCC is not merely
> named "gcc", but e.g. "x86_64-linux-gnu-gcc-7"), which end up naming the
> C_COMPILER_NAME and CP_COMPILER_NAME defines with the specified prefix
> and suffix. Therefore, the regexp being used to match the compiler name
> is wrong because it doesn't take into account the fact that the defines
> may already contain the triplets.
As discussed on IRC, I think the problem is that C_COMPILER_NAME
in libcc1 includes the full triplet in the first place. I think
that it shouldn't. I think that C_COMPILER_NAME should always
be "gcc".
The problem is in bootstrapping code, before there's a plugin
yet -- i.e.., in the code that libcc1 uses to find the compiler (which
then loads a plugin that libcc1 talks with).
Please bear with me while I lay down my rationale, so that we're
in the same page.
C_COMPILER_NAME seems to include the prefix currently in an attempt
to support cross debugging, or more generically, --enable-targets=all
in gdb, but the whole thing doesn't really work as intended if
C_COMPILER_NAME already includes a target prefix.
IIUC the libcc1/plugin design, a single "libcc1.so" (what gdb loads,
not the libcc1plugin compiler plugin) should work with any compiler in
the PATH, in case you have several in the system. E.g., one for
each arch.
Let me expand.
The idea is that gdb always dlopens "libcc1.so", by that name exactly.
Usually that'll open the libcc1.so installed in the system, e.g.,
"/usr/lib64/libcc1.so", which for convenience was originally built from the
same source tree as the systems's compiler was built. You could force gdb to
load some other libcc1.so, e.g., by tweaking LD_LIBRARY_PATH of course,
but you shouldn't need to.
libcc1.so is responsible for finding a compiler that targets the
architecture of the inferior that the user is debugging in gdb.
E.g., say you're cross debugging for arm-none-eabi, on a
x86-64 Fedora host. GDB knows the target inferior's architecture, and passes
down to (the system) libcc1 a triplet regex like "arm*-*eabi*" or
similar to libcc1,. libcc1 appends "-" + C_COMPILER_NAME to that regex,
generating something like "arm*-*eabi*-gcc", and then looks for binaries
in PATH that match that regex. When one is found, e.g., "arm-none-eabi-gcc",
libcc1 forks/execs that compiler, passing it "-fplugin=libcc1plugin".
libcc1 then communicates with that compiler's libcc1plugin plugin
via a socket.
In this scheme, "libcc1.so", the library that gdb loads, has no
target-specific logic at all. It should work with any compiler
in the system, for any target/arch. All it does is marshall the gcc/gdb
interface between the gcc plugin and gdb, it is not linked against gcc.
That boundary is versioned, and ABI-stable. So as long as the
libcc1.so that gdb loads understands the same API version of the gcc/gdb
interface API as gdb understands, it all should work. (The APIs
are always extended keeping backward compatibility.)
So in this scheme, having the "C_COMPILER_NAME" macro in libcc1
include the target prefix for the --target that the plugin that
libcc1 is built along with, seems to serve no real purpose, AFAICT.
It's just getting in the way.
I.e., something like:
"$gdb_specified_triplet_re" + "-" + C_COMPILER_NAME
works if C_COMPILER_NAME is exactly "gcc", but not if C_COMPILER_NAME is
already:
"$whatever_triplet_libcc1_happened_to_be_built_with" + "-gcc"
because we end up with:
"$gdb_specified_triplet_re" + "-"
"$whatever_triplet_libcc1_happened_to_be_built_with" + "-gcc"
which is the problem case.
In sum, I think the libcc1.so (not the plugin) should _not_ have baked
in target awareness, and thus C_COMPILER_NAME should always be "gcc", and
then libcc1's regex should be adjusted to also tolerate a suffix in
the final compiler binary name regex.
WDYT?
Thanks,
Pedro Alves
Re: [libcc1] Improve detection of triplet on compiler names
On 08/23/2017 02:07 PM, Sergio Durigan Junior wrote: > On Wednesday, August 23 2017, Pedro Alves wrote: > >> On 08/23/2017 05:17 AM, Sergio Durigan Junior wrote: >>> The GCC patch improves the libcc1::compiler_triplet_regexp::find and >>> libcp1::compiler_triplet_regexp::find methods by first trying to match >>> the triplet in the compiler name and correctly discarding the triplet >>> part of the regexp if the matching succeeds. I've had to do a few >>> modifications on the way the regexp's are built, but I'll explain them >>> in the patch itself. >>> >>> The GDB patch is very simple: it adds the trailing "-" in the triplet >>> regexp. Therefore, we will have a regexp that truly matches the full >>> triplet (e.g., "^(x86_64|i.86)(-[^-]*)?-linux(-gnu)?-") instead of one >>> that leaves the trailing "-" match to libcc1. >>> >>> I've tested this patch both on my Fedora and my Debian machines, and >>> both now work as expected, independently of the presence of the triplet >>> string in the compiler name. I am sorry about the cross-post, but these >>> patches are really dependent on one another. >> >> Is there a backward/forward compatibility impact? > > Unfortunately, yes. > >> Does new GDB work with old GCC? > > No. On Fedora systems, you would get: > > Could not find a compiler matching > "^(x86_64|i.86)(-[^-]*)?-linux(-gnu)?--gcc$" > That's a problem then. Please read this: https://sourceware.org/gdb/wiki/GCCCompileAndExecute#How_to_extend_the_gdb.2BAC8-gcc_interface > As can be seen, these failures are now happening because of the trailing > dash that is now included in the triplet regexp by GDB. I don't know if > that warrants a change in the API, though. Thanks, Pedro Alves
Re: [libcc1] Improve detection of triplet on compiler names
On 08/23/2017 05:17 AM, Sergio Durigan Junior wrote: > The GCC patch improves the libcc1::compiler_triplet_regexp::find and > libcp1::compiler_triplet_regexp::find methods by first trying to match > the triplet in the compiler name and correctly discarding the triplet > part of the regexp if the matching succeeds. I've had to do a few > modifications on the way the regexp's are built, but I'll explain them > in the patch itself. > > The GDB patch is very simple: it adds the trailing "-" in the triplet > regexp. Therefore, we will have a regexp that truly matches the full > triplet (e.g., "^(x86_64|i.86)(-[^-]*)?-linux(-gnu)?-") instead of one > that leaves the trailing "-" match to libcc1. > > I've tested this patch both on my Fedora and my Debian machines, and > both now work as expected, independently of the presence of the triplet > string in the compiler name. I am sorry about the cross-post, but these > patches are really dependent on one another. Is there a backward/forward compatibility impact? Does new GDB work with old GCC? Does old GDB work with new GCC? Thanks, Pedro Alves
Re: [PATCH] Add macro DISABLE_COPY_AND_ASSIGN
On 08/02/2017 12:19 PM, Yao Qi wrote: > On Wed, Jul 26, 2017 at 9:55 AM, Yao Qi <qiyao...@gmail.com> wrote: >> On 17-07-19 10:30:45, Yao Qi wrote: >>> We have many classes that copy cotr and assignment operator are deleted >>> in different projects, gcc, gdb and gold. So this patch adds a macro >>> to do this, and replace these existing mechanical code with macro >>> DISABLE_COPY_AND_ASSIGN. >>> >>> The patch was posted in gdb-patches, >>> https://sourceware.org/ml/gdb-patches/2017-07/msg00254.html but we >>> think it is better to put this macro in include/ansidecl.h so that >>> other projects can use it too. >>> >>> Boostrapped on x86_64-linux-gnu. Is it OK? >>> >>> include: >>> >>> 2017-07-19 Yao Qi <yao...@linaro.org> >>> Pedro Alves <pal...@redhat.com> >>> >>> * ansidecl.h (DISABLE_COPY_AND_ASSIGN): New macro. >>> >>> gcc/cp: >>> >>> 2017-07-19 Yao Qi <yao...@linaro.org> >>> >>> * cp-tree.h (class ovl_iterator): Use DISABLE_COPY_AND_ASSIGN. >>> * name-lookup.c (struct name_loopup): Likewise. >>> >>> gcc: >>> >>> 2017-07-19 Yao Qi <yao...@linaro.org> >>> >>> * tree-ssa-scopedtables.h (class avail_exprs_stack): Use >>> DISABLE_COPY_AND_ASSIGN. >>> (class const_and_copies): Likewise. >> >> Ping. >> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01134.html >> > > Ping. It is a quite straightforward patch, can any one > take a look? > Yeah, this is a macro that lots of projects out there reinvent, can't imagine it being very controversial. I could have used this today in another spot in gdb. The patch as is touches areas with different maintainers, it may have fallen victim of diffusion of responsibility. Could we get at least the ansidecl.h change in, so we can start using it in gdb? CCing Ian as a libiberty maintainer. Thanks, Pedro Alves
Re: [PATCH 6/6] qsort comparator consistency checking
On 08/03/2017 05:23 PM, Alexander Monakov wrote: > Note that with vec::qsort -> vec::sort renaming (which should be less > controversial, STL also has std::vector::sort), the argument counting > trick won't be needed, the redirection will simply be: OTOH, std::sort's comparison function callback has a different interface from qsort's. std::sort wants less-than true/false, while qsort wants -1/0/1. Might be less confusing to leave "sort" for a method that follows std::sort's interface. You could also consider using std::sort, btw. I don't think there's a reason it can't work with vec. Since std::sort is a template, the inlining + indirection avoidance often results in faster sorts (potentially at the expense of compile time). Consistency checking could be implemented by adding a a gcc::sort wrapper around std::sort (and calling the former throughout). Thanks, Pedro Alves
Re: [PATCH] Add -std=c++2a
On 07/20/2017 02:33 PM, Andrew Sutton wrote:
> This adds a new C++ dialect, enabled by -std=c++2a.
Hi Andrew,
I happened to skim this patch and notice a couple issues.
See below.
> +/* Set the C++ 202a draft standard (without GNU extensions if ISO). */
> +static void
> +set_std_cxx2a (int iso)
> +{
> + cpp_set_lang (parse_in, iso ? CLK_CXX2A: CLK_GNUCXX2A);
> + flag_no_gnu_keywords = iso;
> + flag_no_nonansi_builtin = iso;
> + flag_iso = iso;
> + /* C++1z includes the C99 standard library. */
> + flag_isoc94 = 1;
> + flag_isoc99 = 1;
> + flag_isoc11 = 1;
> + cxx_dialect = cxx2a;
> + lang_hooks.name = "GNU C++17"; /* Pretend C++17 until standardization. */
Did you mean to write C++20 here?
> --- a/libcpp/include/cpplib.h
> +++ b/libcpp/include/cpplib.h
> @@ -171,7 +171,8 @@ enum cpp_ttype
> enum c_lang {CLK_GNUC89 = 0, CLK_GNUC99, CLK_GNUC11,
>CLK_STDC89, CLK_STDC94, CLK_STDC99, CLK_STDC11,
>CLK_GNUCXX, CLK_CXX98, CLK_GNUCXX11, CLK_CXX11,
> - CLK_GNUCXX14, CLK_CXX14, CLK_GNUCXX1Z, CLK_CXX1Z, CLK_ASM};
> + CLK_GNUCXX14, CLK_CXX14, CLK_GNUCXX1Z, CLK_CXX1Z,
> + CLK_GNUCXX2A, CLK_CXX2A, CLK_ASM};
Tabs vs spaces?
> @@ -497,7 +499,10 @@ cpp_init_builtins (cpp_reader *pfile, int hosted)
>
>if (CPP_OPTION (pfile, cplusplus))
> {
> - if (CPP_OPTION (pfile, lang) == CLK_CXX1Z
> + if (CPP_OPTION (pfile, lang) == CLK_CXX2A
> + || CPP_OPTION (pfile, lang) == CLK_GNUCXX2A)
> + _cpp_define_builtin (pfile, "__cplusplus 201707L");
I think you wanted 202007L here.
> + else if (CPP_OPTION (pfile, lang) == CLK_CXX1Z
> || CPP_OPTION (pfile, lang) == CLK_GNUCXX1Z)
> _cpp_define_builtin (pfile, "__cplusplus 201703L");
>else if (CPP_OPTION (pfile, lang) == CLK_CXX14
>
Thanks,
Pedro Alves
Re: [PATCH 0/2] add unique_ptr class
On 08/04/2017 07:52 PM, Jonathan Wakely wrote: > On 31/07/17 19:46 -0400, tbsaunde+...@tbsaunde.org wrote: >> I've been saying I'd do this for a long time, but I'm finally getting to >> importing the C++98 compatable unique_ptr class Pedro wrote for gdb a >> while >> back. Thanks a lot for doing this! > I believe the gtl namespace also comes from Pedro, but GNU template > library seems as reasonable as any other name I can come up with. Yes, I had suggested it here: https://sourceware.org/ml/gdb-patches/2017-02/msg00197.html > > If it's inspired by "STL" then can we call it something else? > > std::unique_ptr is not part of the STL, because the STL is a library > of containers and algorithms from the 1990s. std::unique_ptr is > something much newer that doesn't originate in the STL. > > STL != C++ Standard Library That I knew, but gtl was not really a reference to the C++ Standard Library, so I don't see the problem. It was supposed to be the name of a library which would contain other C++ utilities that would be shared by different GNU toolchain components. Some of those bits would be inspired by, or be straight backports of more-recent standards, but it'd be more than that. An option would be to keep "gtl" as acronym, but expand it to "GNU Toolchain Library" instead. For example, gdb has been growing C++ utilities under gdb/common/ that might be handy for gcc and other projects too, for example: - enum_flags (type-safe enum bit flags) - function_view (non-owning reference to callables) - offset_type (type safe / distinct integer types to represent offsets into anything addressable) - optional (C++11 backport of libstdc++'s std::optional) - refcounted_object.h (intrusively-refcounted types) - scoped_restore (RAII save/restore of globals) - an allocator that default-constructs using default-initialization instead of value-initialization. and more. GCC OTOH has code that might be handy for GDB as well, like for example the open addressing hash tables (hash_map/hash_table/hash_set etc.). Maybe Gold could make use of some of this code too. There are some bits in Gold that might be useful for (at least) GDB too. For example, its Stringpool class. I think there's a lot of scope for sharing more code between the different components of the GNU toolchain, even beyond general random utilities and data structures, and I'd love to see us move more in that direction. > If we want a namespace for GNU utilities, what's wrong with "gnu"? That'd be an "obvious" choice, and I'm not terribly against it, though I wonder whether it'd be taking over a name that has a wider scope than intended? I.e., GNU is a larger set of projects than the GNU toolchain. For example, there's Gnulib, which already compiles as libgnu.a / -lgnu, which might be confusing. GCC doesn't currently use Gnulib, but GDB does, and, there was work going on a while ago to make GCC use gnulib as well. Thanks, Pedro Alves
Re: [PATCH] Remove Pascal language in source code.
On 07/13/2017 03:59 PM, Jeff Law wrote: > The only concern I'd have here is the bits in dbxout.[ch] might > effectively be the best documentation of the dbxout format that exists. > Thus, dropping something like N_SO_PASCAL loses that historical > documentation. FYI, there's a texinfo document in the GDB repo describing the stabs format, and it documents N_SO_PASCAL: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/doc/stabs.texinfo;h=a7ea808a41b290b7dfc4f44801a540a834ee04db;hb=HEAD#l440 A pre-generated html version is live here: https://www.sourceware.org/gdb/onlinedocs/stabs.html https://www.sourceware.org/gdb/onlinedocs/stabs.html#Source-Files Thanks, Pedro Alves
Re: [PATCH] Finish implementing P0426R1 "Constexpr for std::char_traits" for C++17
On 06/05/2017 03:27 PM, Jonathan Wakely wrote:
> Pedro, this is OK for trunk now we're in stage 1. Please go ahead and
> commit it - thanks.
Thanks Jonathan. I've pushed it in now.
>
> It's probably safe for gcc-7-branch too, but let's leave it on trunk
> for a while first.
OK.
BTW, for extra thoroughness, to confirm we're handling both
const & non-const arrays correctly, I had written this testsuite
tweak too. Would you like to have this in?
>From 3f7adab8bab68955aafd760467bb860057140d40 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pal...@redhat.com>
Date: Mon, 12 Jun 2017 20:23:23 +0100
Subject: [PATCH] constexpr char_traits: Test non-const strings/arrays too
libstdc++-v3/ChangeLog
-mm-dd Pedro Alves <pal...@redhat.com>
*
testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
(test_assign, test_compare, test_length, test_find): Test
non-const strings/arrays too.
(struct C): Add a generic conversion ctor.
---
.../requirements/constexpr_functions_c++17.cc | 173 ++---
1 file changed, 150 insertions(+), 23 deletions(-)
diff --git
a/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
b/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
index efd280f..c41b490 100644
---
a/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
+++
b/libstdc++-v3/testsuite/21_strings/char_traits/requirements/constexpr_functions_c++17.cc
@@ -25,10 +25,40 @@ template
test_assign()
{
using char_type = typename CT::char_type;
-char_type s1[2] = {};
-const char_type s2[2] = {1, 0};
-CT::assign(s1[0], s2[0]);
-return s1[0] == char_type{1};
+
+auto check = [](char_type* s1, const char_type* s2)
+ {
+ CT::assign(s1[0], s2[0]);
+ return s1[0] == char_type{1};
+ };
+
+// const strings.
+
+{
+ char_type s1[2] = {};
+ const char_type s2[2] = {1, 0};
+ if (!check (s1, s2))
+ return false;
+}
+
+// non-const strings.
+
+{
+ char_type s1[2] = {};
+ char_type s2[2] = {1, 0};
+ if (!check (s1, s2))
+ return false;
+}
+
+{
+ char_type s1[2] = {};
+ char_type s2[2] = {};
+ s2[0] = char_type{1};
+ if (!check (s1, s2))
+ return false;
+}
+
+return true;
}
template
@@ -36,14 +66,48 @@ template
test_compare()
{
using char_type = typename CT::char_type;
-const char_type s1[3] = {1, 2, 3};
-const char_type s2[3] = {1, 2, 3};
-if (CT::compare(s1, s2, 3) != 0)
- return false;
-if (CT::compare(s2, s1, 3) != 0)
- return false;
-if (CT::compare(s1+1, s2, 2) <= 0)
- return false;
+
+auto check = [](const char_type* s1, const char_type* s2)
+ {
+ if (CT::compare(s1, s2, 3) != 0)
+ return false;
+ if (CT::compare(s2, s1, 3) != 0)
+ return false;
+ if (CT::compare(s1+1, s2, 2) <= 0)
+ return false;
+ return true;
+ };
+
+// const arrays.
+
+{
+ const char_type s1[3] = {1, 2, 3};
+ const char_type s2[3] = {1, 2, 3};
+ if (!check (s1, s2))
+ return false;
+}
+
+// non-const arrays.
+
+{
+ char_type s1[3] = {1, 2, 3};
+ char_type s2[3] = {1, 2, 3};
+ if (!check (s1, s2))
+ return false;
+}
+
+{
+ char_type s1[3] = {};
+ char_type s2[3] = {};
+ for (size_t i = 0; i < 3; i++)
+ {
+ s1[i] = char_type(i+1);
+ s2[i] = char_type(i+1);
+ }
+ if (!check (s1, s2))
+ return false;
+}
+
return true;
}
@@ -52,11 +116,40 @@ template
test_length()
{
using char_type = typename CT::char_type;
-const char_type s1[4] = {1, 2, 3, 0};
-if (CT::length(s1) != 3)
- return false;
-if (CT::length(s1+1) != 2)
- return false;
+
+auto check = [](const char_type* s)
+ {
+ if (CT::length(s) != 3)
+ return false;
+ if (CT::length(s+1) != 2)
+ return false;
+ return true;
+ };
+
+// const strings.
+
+{
+ const char_type s[4] = {1, 2, 3, 0};
+ if (!check (s))
+ return false;
+}
+
+// non-const strings.
+
+{
+ char_type s[4] = {1, 2, 3, 0};
+ if (!check (s))
+ return false;
+}
+
+{
+ char_type s[4] = {};
+ for (size_t i = 0; i < 3; i++)
+ s[i] = char_type(i+1);
+ if (!check (s))
+ return false;
+}
+
return true;
}
@@ -65,11 +158,40 @@ template
test_find()
{
using char_type = typename CT::char_type;
-const char_type s1[3] = {1, 2, 3};
-if (CT::find(s1, 3, char_type{2}) != s1+1)
- return false;
-if (CT::find(s1, 3, char_type{4}) != nullptr)
- return false;
+
+auto check = [](const char_type* s)
+ {
+ if (CT::find(s, 3
Re: [RFC] Dejagnu patch to handle multi-line directives
On 06/12/2017 08:59 AM, Richard Sandiford wrote:
> I realise there's probably more that can go wrong with it, but how
> about instead treating unbalanced { ... } as a sign that the directive
> continues to the next line? This would allow:
>
> /* { dg-additional-options
> "-DSTACK_SIZE=[dg-effective-target-value stack_size]"
> { target { stack_size } } } */
In a TCL .exp file you'd split the lines with a '\' continuation
character. Wouldn't that be more natural? Like:
/* { dg-additional-options \
"-DSTACK_SIZE=[dg-effective-target-value stack_size]" \
{ target { stack_size } } } */
Might be less magical and simpler to implement too.
Thanks,
Pedro Alves
Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)
On 06/08/2017 12:19 PM, Marek Polacek wrote: > On Thu, Jun 08, 2017 at 12:01:03PM +0100, Pedro Alves wrote: >> On 06/08/2017 11:29 AM, Marek Polacek wrote: >>> On Wed, Jun 07, 2017 at 08:02:42PM +0100, Pedro Alves wrote: >>>> Hi Marek, >>>> >>>> Nice warning! Just to confirm, would the patch warn with code like: >>> >>> Thanks. BTW, if you (or anyone) could come up with a better name, >>> I'm all ears. >> >> AFAICS, the warning's intent is catching the case of a >> a macro expanding to multiple (top level) statements, not lines. > > True. I felt that it was implicitly understood what's meant by that, > but I'll change that. Martin pointed this out, too. I don't think it's implicit, because it could raise questions, like: >> +Wmultiline-expansion >> +C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C >> ObjC C++ ObjC++,Wall) >> +Warn about macros expanding to multiple statements in a body of a >> conditional such as if, else, while, or for. " Hmm, the description doesn't actually describe what is meant by "multiple lines". Does "multiline" here mean that the warning triggers with: #define FOO() \ foo1(); \ foo2() but not #define FOO() foo1(); foo2() ? " It's perhaps obvious to seasoned hackers that that's not the intention, but not all users are experts. So a general rule I try to follow (in GDB) is: make sure that the description of an option matches the option's name. I.e., if the words used to name the option name don't appear in the option's description, then that's a good indication something is off and is bound to confuse someone. >> >> So it'd seem clearer to me if the warning was named around >> "-Wmulti-statement-something" instead of "-Wmultiline-something"? >> >> -Wmulti-statement-expansion >> -Wmulti-statement-macros >> -Wmulti-statement-macro >> -Wmulti-statement-macro-expansion > > I think I'll go with -Wmultistatement-expansion (without the dash). Fine with me, FWIW. > I'll post a new version with the warning renamed and the new test added. Great, thanks much! Thanks, Pedro Alves
Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)
On 06/08/2017 11:29 AM, Marek Polacek wrote:
> On Wed, Jun 07, 2017 at 08:02:42PM +0100, Pedro Alves wrote:
>> Hi Marek,
>>
>> Nice warning! Just to confirm, would the patch warn with code like:
>
> Thanks. BTW, if you (or anyone) could come up with a better name,
> I'm all ears.
AFAICS, the warning's intent is catching the case of a
a macro expanding to multiple (top level) statements, not lines.
Both the comments in the code and the description of the
warning talk in those terms:
+/* () This warning warns about
+ cases when a macro expands to multiple statements not wrapped in
+ do {} while (0) or ({ }) and is used as a body of if/else/for/while
+ conditionals. For example,
+Wmultiline-expansion
+C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC
C++ ObjC++,Wall)
+Warn about macros expanding to multiple statements in a body of a conditional
such as if, else, while, or for.
So it'd seem clearer to me if the warning was named around
"-Wmulti-statement-something" instead of "-Wmultiline-something"?
-Wmulti-statement-expansion
-Wmulti-statement-macros
-Wmulti-statement-macro
-Wmulti-statement-macro-expansion
Particularly when one could argue that "multiline expansion" in
context of macros doesn't make any sense, given macros always
expand to a single line:
#define SAME_LINE \
(__LINE__ \
== __LINE__)
static_assert (SAME_LINE, "");
> Nope, it doesn't warn (neither C nor C++). I should probably add this test.
Thanks for confirming. A test would be nice, to make sure we
don't regress.
Thanks,
Pedro Alves
Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)
Hi Marek,
Nice warning! Just to confirm, would the patch warn with code like:
const char *
target_xfer_status_to_string (enum target_xfer_status status)
{
#define CASE(X) case X: return #X
switch (status)
{
CASE(TARGET_XFER_E_IO);
CASE(TARGET_XFER_UNAVAILABLE);
default:
return "";
}
#undef CASE
};
?
I think it shouldn't, but I couldn't tell from the tests,
and the only similar instance I found in gcc is guarded
behind an #ifdef (in eh_data_format_name).
Thanks,
Pedro Alves
Re: MinGW compilation warnings in libiberty's include/environ.h
On 05/20/2017 02:27 AM, DJ Delorie wrote: > > Pedro Alves <pal...@redhat.com> writes: >> That sounds to me like the root issue that should be fixed, >> so that these fallback definitions don't come into into play at all. >> I.e., why isn't HAVE_ENVIRON_DECL defined on mingw when >> setenv.o is built? Sounds like a decl check is missing >> in configure.ac. > > environ is tricky because it's typically messy on platforms, unlike a > standard C function. You can't use a generic check if the macro expands > to something that interferes with the check. gnulib has a check, which I assume to be solid: http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=m4/environ.m4 we could import just that bit, I suppose, though every client of libiberty's environ.h would need to gain the same check. That's be quite doable with the shared libiberty.m4 idea (we'd just put the check there), but a nuisance if you have to copy the check all over the place. At this point it may be more worth it to invest in finishing the previous use-gnulib-in-gcc effort instead. Thanks, Pedro Alves
Re: MinGW compilation warnings in libiberty's xstrndup.c
On 05/20/2017 01:38 AM, DJ Delorie wrote: > > Pedro Alves <pal...@redhat.com> writes: >> Ah, yeah. AFAICS, all the declaration checks in libiberty.h are >> HAVE_DECL checks. This suggests to me that this declaration guard >> should be HAVE_DECL too [1]. > > Except the ones in the $funcs list, which includes strnlen. I think in > the old days, we didn't put in declarations at all... until "char *" > became a different size than "int" and we started needing them. Running: $ grep HAVE_ libiberty/config.h | sed 's/DECL_//g'| sort | uniq -c | sort -n on the build I have handy shows: ... 2 #define HAVE_ASPRINTF 1 2 #define HAVE_BASENAME 1 2 #define HAVE_CALLOC 1 2 #define HAVE_FFS 1 2 #define HAVE_SBRK 1 2 #define HAVE_SNPRINTF 1 2 #define HAVE_STRTOL 1 2 #define HAVE_STRTOLL 1 2 #define HAVE_STRTOUL 1 2 #define HAVE_STRTOULL 1 2 #define HAVE_STRVERSCMP 1 2 #define HAVE_VASPRINTF 1 "2" means above means each FOO symbol above has both HAVE_FOO and HAVE_DECL_FOO defines: $ grep "HAVE.*_SNPRINTF" config.h #define HAVE_DECL_SNPRINTF 1 #define HAVE_SNPRINTF 1 > > So some functions in libiberty are HAVE_DECL and others are still HAVE. But I don't see any HAVE check in libiberty.h (for function symbols), only HAVE_DECL ones: $ grep HAVE libiberty.h /* HAVE_DECL_* is a three-state macro: undefined, 0 or 1. If it is #if !HAVE_DECL_BASENAME || defined (__DragonFly__) || defined (HAVE_DECL_BASENAME) autoconf which would result in HAVE_DECL_BASENAME being set. */ #if defined (HAVE_DECL_FFS) && !HAVE_DECL_FFS #if defined(HAVE_DECL_ASPRINTF) && !HAVE_DECL_ASPRINTF #if !HAVE_DECL_VASPRINTF #if defined(HAVE_DECL_SNPRINTF) && !HAVE_DECL_SNPRINTF #if defined(HAVE_DECL_VSNPRINTF) && !HAVE_DECL_VSNPRINTF #if defined (HAVE_DECL_STRNLEN) && !HAVE_DECL_STRNLEN #if defined(HAVE_DECL_STRVERSCMP) && !HAVE_DECL_STRVERSCMP #if defined(HAVE_DECL_STRTOL) && !HAVE_DECL_STRTOL #if defined(HAVE_DECL_STRTOUL) && !HAVE_DECL_STRTOUL #if defined(HAVE_LONG_LONG) && defined(HAVE_DECL_STRTOLL) && !HAVE_DECL_STRTOLL #if defined(HAVE_LONG_LONG) && defined(HAVE_DECL_STRTOULL) && !HAVE_DECL_STRTOULL #if defined(HAVE_DECL_STRVERSCMP) && !HAVE_DECL_STRVERSCMP Nor in other headers under include/, while at it. Are you looking elsewhere perhaps? Based on the above, it looks to me like the non-HAVE_DECL HAVE symbols are implementation detail to libiberty, side effect of the checks used to determine whether a replacement is necessary. > > Ah, found it, this commit is incomplete: > > https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00784.html > > It changes gcc's configure but nobody else's (and now we have an answer > to the three-year-old question "why don't we have a more liberal commit > policy?" ;) which breaks both libiberty and libgfortran. Yeah, that exactly the sort of thing that gets fixed by design by having a centralized libiberty.m4 file. > >> BTW, I once proposed a new libiberty.m4 file that all libiberty >> clients would source so that these checks are all centralized. > > I have no philosophical problem with that type of change, but I have the > usual fear of touching anything in libiberty that's been around this > long ;-) > > (this bug being a prime example of how subtle an incorrect change can be) > > (and honestly, my upstream attention is elsewhere these days) > Thanks, Pedro Alves
Re: MinGW compilation warnings in libiberty's xstrndup.c
On 05/19/2017 11:31 PM, DJ Delorie wrote: > > Right, I meant, libiberty's configure, gcc's configure, binutils' > configure, and gdb's configure, all need to agree on whether strnlen is > a HAVE or a HAVE_DECL type symbol. If they don't, the header can't > provide "one" working solution. > Ah, yeah. AFAICS, all the declaration checks in libiberty.h are HAVE_DECL checks. This suggests to me that this declaration guard should be HAVE_DECL too [1]. BTW, I once proposed a new libiberty.m4 file that all libiberty clients would source so that these checks are all centralized. Here: https://gcc.gnu.org/ml/gcc-patches/2015-02/msg00580.html And follow up here: https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01712.html Leading to (as, gold, ld, gdb and libiberty/ itself converted): https://github.com/palves/gdb/commits/palves/libiberty_m4 I never tried adjusting gcc, but even if it wouldn't work there, it'd still be a net win. Wonder what others think of that approach. Thanks, Pedro Alves
Re: MinGW compilation warnings in libiberty's xstrndup.c
On 05/19/2017 10:56 PM, DJ Delorie wrote: > > Eli Zaretskii <e...@gnu.org> writes: >> It should use HAVE_STRNLEN instead, because that's the only >> strnlen-related macro defined in config.g when strnlen is probed by >> the configure script. > > Ah, but gcc's configure defines HAVE_DECL_STRNLEN. Header guards need > to be coordinated across all the users, not just libiberty. > The "user" in this case is libiberty itself. Thanks, Pedro Alves
Re: MinGW compilation warnings in libiberty's xstrndup.c
On 05/19/2017 04:40 PM, Eli Zaretskii wrote: >> Cc: gdb-patc...@sourceware.org, Thomas Schwinge <tho...@codesourcery.com> >> From: Pedro Alves <pal...@redhat.com> >> Date: Fri, 19 May 2017 16:22:55 +0100 >> >> But then, xstrndup.c has at the top: >> >> #ifdef HAVE_STRING_H >> #include >> #else >> # ifdef HAVE_STRINGS_H >> # include >> # endif >> #endif >> >> So I would expect your build to pick the strnlen declaration from >> one of the string.h or strings.h mingw headers. Why didn't it? > > Because MinGW doesn't have it, not unless you build a program that > will require one of the newer versions of the Windows C runtime > library. That's why libiberty's strnlen is being compiled in the > MinGW build in the first place. OK, I didn't realize there was a strnlen replacement too. > > Specifically, the MinGW headers do provide a prototype for strnlen if > the program defines __MSVCRT_VERSION__ to be a high enough version, or > defines _POSIX_C_SOURCE >= 200809L, but none of these is set by > default, and is not a good idea, as explained above, for a program > that needs to run on a wide variety of OS versions. > > IOW, libiberty shouldn't rely on the system headers to provide a > strnlen prototype when libiberty's strnlen is being included in the > library as a replacement. OK, I guess then we're up to figuring out which direction to go. Either an AC_CHECK_DECL is missing on libiberty's configure, or the original patch really wanted AC_CHECK_FUNC instead of AC_CHECK_DECL. Or something else, I only look at libiberty's configury every couple of years and forget how this is all supposed to work in between. Thanks, Pedro Alves
Re: MinGW compilation warnings in libiberty's include/environ.h
On 05/08/2017 04:25 PM, Eli Zaretskii wrote: > When compiling libiberty (as part of GDB) with MinGW on MS-Windows, I > see the following warning: > > gcc -c -DHAVE_CONFIG_H -O2 -gdwarf-4 -g3 -D__USE_MINGW_ACCESS -I. > -I./../include -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes > -pedantic -D_GNU_SOURCE ./setenv.c -o setenv.o > In file included from ./setenv.c:64:0: > ./../include/environ.h:30:1: warning: function declaration isn't a > prototype [-Wstrict-prototypes] > extern char **environ; > ^ > > This was already reported 4 years ago, here: > > https://gcc.gnu.org/ml/gcc-patches/2013-03/msg00471.html > > and was solved back then. But it looks like the offending code was > copied to include/environ.h without the fix, and the warning is thus > back. > > The problem is with this code in environ.h: > > #ifndef HAVE_ENVIRON_DECL > # ifdef __APPLE__ > # include > # define environ (*_NSGetEnviron ()) > # else > extern char **environ; > # endif > # define HAVE_ENVIRON_DECL > #endif > > which causes the MinGW compiler to see the declaration of environ, > whereas MinGW's stdlib.h has this: > > #ifdef __MSVCRT__ > # define _environ (*__p__environ()) > extern _CRTIMP __cdecl __MINGW_NOTHROW char ***__p__environ(void); > # define _wenviron (*__p__wenviron()) > extern _CRTIMP __cdecl __MINGW_NOTHROW wchar_t ***__p__wenviron(void); > > #else /* ! __MSVCRT__ */ > # ifndef __DECLSPEC_SUPPORTED > # define _environ (*_imp___environ_dll) > extern char ***_imp___environ_dll; > > # else /* __DECLSPEC_SUPPORTED */ > # define _environ _environ_dll > __MINGW_IMPORT char ** _environ_dll; > # endif /* __DECLSPEC_SUPPORTED */ > #endif /* ! __MSVCRT__ */ > > #define environ _environ So again there's a system header that defines the symbol but for some reason libiberty still wants to declare/define it is if it weren't? That sounds to me like the root issue that should be fixed, so that these fallback definitions don't come into into play at all. I.e., why isn't HAVE_ENVIRON_DECL defined on mingw when setenv.o is built? Sounds like a decl check is missing in configure.ac. Thanks, Pedro Alves
Re: MinGW compilation warnings in libiberty's waitpid.c
On 05/08/2017 04:27 PM, Eli Zaretskii wrote: > When compiling libiberty (as part of GDB) with MinGW on MS-Windows, I > see the following warning: > > gcc -c -DHAVE_CONFIG_H -O2 -gdwarf-4 -g3 -D__USE_MINGW_ACCESS -I. > -I./../include -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes > -pedantic -D_GNU_SOURCE ./waitpid.c -o waitpid.o > ./waitpid.c: In function 'waitpid': > ./waitpid.c:31:18: warning: implicit declaration of function 'wait' > [-Wimplicit-function-declaration] > int wpid = wait(stat_loc); > ^ > > The file waitpid.c should not be built on MinGW, as it is not needed > on Windows, and will not work if the function is called (because > there's no 'wait' function on MS-Windows). > Makes sense, but did you check whether there's an obvious place such a change could be done in configure.ac? I wonder what code relies on this replacement, actually. In liberty, the only waitpid calls are in pex-unix.c, but those are all guarded by HAVE_WAITPID. Maybe it was used at some point when libiberty was a target library? So I wonder whether we could just unconditionally remove the waitpid replacement instead. Thanks, Pedro Alves
Re: MinGW compilation warnings in libiberty's xstrndup.c
[Adding Thomas] On 05/08/2017 04:30 PM, Eli Zaretskii wrote: > When compiling libiberty (as part of GDB) with MinGW on MS-Windows, I > see the following warning: > > gcc -c -DHAVE_CONFIG_H -O2 -gdwarf-4 -g3 -D__USE_MINGW_ACCESS -I. > -I./../include -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes > -pedantic -D_GNU_SOURCE ./xstrndup.c -o xstrndup.o > ./xstrndup.c: In function 'xstrndup': > ./xstrndup.c:51:16: warning: implicit declaration of function 'strnlen' > [-Wimplicit-function-declaration] > size_t len = strnlen (s, n); >^ > > This happens because libiberty.h uses incorrect guards for the > prototype of strnlen: > > #if defined (HAVE_DECL_STRNLEN) && !HAVE_DECL_STRNLEN > extern size_t strnlen (const char *, size_t); > #endif > > It should use HAVE_STRNLEN instead, because that's the only > strnlen-related macro defined in config.g when strnlen is probed by > the configure script. Looks like the declaration check got added to gcc's configure, but not elsewhere, with: commit d968efeac356364c01445013a1a3660e5087cb15 Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4> AuthorDate: Tue Jun 10 09:45:00 2014 + [PR lto/61334] Declare prototype for strnlen, if needed. include/ * libiberty.h [defined (HAVE_DECL_STRNLEN) && !HAVE_DECL_STRNLEN] (strnlen): New prototype. gcc/ * configure.ac: Use gcc_AC_CHECK_DECLS to check for strnlen prototype. * config.in: Regenerate. * configure: Likewise. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@211401 138bc75d-0d04-0410-961f-82ee72b054a4 But then, xstrndup.c has at the top: #ifdef HAVE_STRING_H #include #else # ifdef HAVE_STRINGS_H # include # endif #endif So I would expect your build to pick the strnlen declaration from one of the string.h or strings.h mingw headers. Why didn't it? Thanks, Pedro Alves
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
[Meant to add this to the other email, but forgot to copy it back.]
On 05/12/2017 03:31 AM, Martin Sebor wrote:
> + // Zeroing out is diagnosed only because it's difficult not to.
> + // Otherwise, a class that's non-trivial only because it has
> + // a non-trivial dtor can be safely zeroed out (that's what
> + // default-initializing it does).
FWIW, I found references like these to "default initialization" confusing,
until I realized you're likely talking about default initialization in
pre-C++11 terms. I.e., "new T()" with parens. I first assumed you're
talking about default initialization like "new T;" or "T t;", but in
that case the object would not the zeroed out, hence the confusion.
It may be beneficial to go over the comments in the patch (and
particularly documentation) and talk in C++11 (and later)
terms -- i.e., say "value-initializing it" instead.
> + T (bzero, (p, sizeof *p));// { dg-warning "bzero" }
> + T (memset, (p, 0, sizeof *p));// { dg-warning "memset" }
> + T (memset, (p, i, sizeof *p));// { dg-warning "memset" }
Thanks,
Pedro Alves
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
On 05/17/2017 02:55 AM, Martin Sebor wrote: > On 05/16/2017 04:48 PM, Pedro Alves wrote: >> On 05/16/2017 08:41 PM, Jason Merrill wrote: >> >>> I agree that it makes sense to >>> check for a trivial assignment operator specifically. I guess we want >>> a slightly stronger "trivially copyable" that also requires a >>> non-deleted assignment operator. >>> >>> It seems to me that the relevant tests are: >>> >>> bcopy/memcpy/memmove want trivally copyable + non-deleted assignment. >>> bzero/memset want trivial + non-deleted assignment. >>> >>> I'm still not convinced we need to consider standard-layout at all. >> >> What do you think of warning for memset of types with references? Having slept, I now realize you had that covered already by the "non-deleted assignment" requirement... A reference data member makes the assignment operator be implicitly deleted. Sorry for the noise. >> While at it, maybe the same reasoning would justify warn of memcpy/memset >> of types with const data members? Ditto. > I did this because objects with references cannot be assigned > to (the default copy assignment is deleted). So as a baseline > rule, I made the warning trigger whenever a native assignment > or copy isn't valid. In the IMO unlikely event that a memcpy > over a reference is intended, the warning is easy to suppress. Agreed. I wondered whether we'll end up wanting to distinguish these cases: #1memcpy (T *, const T *src, size_t n); #2.1 memcpy (T *, const char *src, size_t n); // char, void, std::byte... #2.2 memcpy (char *, const T *src, size_t n); // char, void, std::byte... #3memcpy (T *, const U *src, size_t n); Where: - T is a trivially copyable type that still triggers the new warning. - U is a type unrelated to T, and is not (unsigned) char, void or std::byte. #1 is the case that looks like copy. #2.1 and 2.2 may well appear in type-erasing code. #3 would look like a way to work around aliasing issues and (even though ISTR that it's OK as GNU extension if T and U are trivial) worthy of a warning even if T is trivial under the definition of the warning. Reading your updated patch, I see that you warn already when T is trivial and U is not trivial, but IIUC, not if U is also trivial, even if unrelated to T. Anyway, I don't really want to argue about this -- I started writing this paragraph before actually reading the patch, and then actually read the patch and was pleasantly surprised with what I saw. I think it's looking great. > I used a similar (though not exactly the same) rationale for > warning for const members. They too cannot be assigned to, > and letting memset or memcpy silently change them violates > const-correctnes. *Nod* > It's also undefined I'm not sure, I think there may be nuances, as usual. AFAICS, it's generally valid to memcpy into trivially copyable types that have const members to/from untyped/char storage, AFAICS. Might need to apply std::launder afterwards. But it isn't clear to me, since whether memcpy starts a (trivial) object's lifetime is up in the air, AFAIK. But I'm not suggesting to really consider that. The rare specialized code will be able to work around the spurious warning. > and the immutability > of such members an optimization opportunity waiting to be > exploited. > *nod* >>> +Wnon-trivial-memaccess >>> +C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ >>> ObjC++, Wall) >>> +Warn for raw memory writes to objects of non-trivial types. >> >> May I suggest renaming the warning (and the description above) from >> -Wnon-trivial-memaccess to something else that avoids "trivial" in >> its name? Because it's no longer strictly about "trivial" in the >> standard C++ sense. The documentation talks about "The safe way", >> and "does not warn on safe calls", so maybe call it -Wunsafe-memaccess? > > I debated whether to rename things (not just the warning but > also the function that implements it in GCC). Ultimately I > decided it wasn't necessary because the rules seem close enough > to be based on some notion of triviality and because no better > name came to mind. -Wunsafe-memaccess might work. The one mild > concern I have with it is that it could suggest it might do more > than simple type checking (e.g., buffer overflow and what not). > Let's see what Jason thinks. Yet another motivation of avoiding "trivial" that crossed my mind is that you may want to enable the warning in C too, e.g., for warning about the memcpy of types with const members. But C as no concept of "trivial", everything is "trivial". >> (I spotted a couple typos in the new patch: "otherwse", "becase", btw.) > > I'm a horrible typist. I'll proofread the patch again and fix > them up before committing it. Thanks much for working on this. I think this will uncover lots of latent bugs in many codebases. Looking forward to have this in. Thanks, Pedro Alves
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
On 05/16/2017 08:41 PM, Jason Merrill wrote: > I agree that it makes sense to > check for a trivial assignment operator specifically. I guess we want > a slightly stronger "trivially copyable" that also requires a > non-deleted assignment operator. > > It seems to me that the relevant tests are: > > bcopy/memcpy/memmove want trivally copyable + non-deleted assignment. > bzero/memset want trivial + non-deleted assignment. > > I'm still not convinced we need to consider standard-layout at all. What do you think of warning for memset of types with references? Since NULL references are undefined behavior (N4659, [dcl.ref]/5), IMHO such code is quite smelly and most likely a sign of incomplete refactoring, not design. (My original intention for poisoning memset of standard-layout type in gdb was mainly as proxy/approximation for "type with references". Other properties that make a type non-standard-layout are not as interesting to me.) While at it, maybe the same reasoning would justify warn of memcpy/memset of types with const data members? memcpy of such types kind of sounds like a recipe for subtle breakage that could only be salvaged with std::launder. And if you know about that, you'll probably be copying objects of type T to/from raw byte/char storage, not to/from another T. Actually, now that I look at Martin's new patch, I see he's already warning about const data members and references, both memcpy and memset. I'm not super convinced on warning about memcpy of references (unlike memset), but I don't feel strongly against it either. I'd be fine (FWIW) with giving it a try and see what breaks out there. > +Wnon-trivial-memaccess > +C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ ObjC++, > Wall) > +Warn for raw memory writes to objects of non-trivial types. May I suggest renaming the warning (and the description above) from -Wnon-trivial-memaccess to something else that avoids "trivial" in its name? Because it's no longer strictly about "trivial" in the standard C++ sense. The documentation talks about "The safe way", and "does not warn on safe calls", so maybe call it -Wunsafe-memaccess? (I spotted a couple typos in the new patch: "otherwse", "becase", btw.) Thanks, Pedro Alves
Re: [1/2] PR 78736: New warning -Wenum-conversion
On 05/09/2017 07:04 PM, Martin Sebor wrote:
>>>
>
> -Wassign-enum is a Clang warning too, it just isn't included in
> either -Wall or -Wextra. It warns when a constant is assigned
> to a variable of an enumerated type and is not representable in
> it. I enhanced it for GCC to also warn when the constant doesn't
> correspond to an enumerator in the type,
Note that that means that the warning will trigger in the common use
case of using enums as bit flags, like:
enum flags
{
F1 = 1 << 1,
F2 = 1 << 2,
F3 = 1 << 3
};
void foo ()
{
enum flags f = 0;
f = F1 | F2;
}
I was going to suggest adding an attribute to mark such enum
types, and it turns out that Clang has one already:
https://clang.llvm.org/docs/AttributeReference.html#flag-enum
So, IMHO if we add the warning, IWBN to add the attribute as well.
> but I'm starting to think
> that rather than adding yet another option to GCC it might be better
> to extend your -Wenum-conversion once it's committed to cover those
> cases (and also to avoid issuing multiple warnings for essentially
> the same problem). Let me ponder that some more.
Thanks,
Pedro Alves
Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)
f trivial_type_p, for
memcpy/memmove/mempcpy at least.
For memset, I'd suggest to go the other direction actually, and
instead of relaxing the trivial check, to tighten it, by warning about
memset'ting objects of non-standard-layout type too. I.e., warn for
memset of all non-POD (non-trivial + non-standard-layout) types. That's
what I did in the gdb patch. That would also produce warnings for memset
of trivial types that via refactoring end up with references, like:
struct Trivial
{
Trivial () = default;
Trivial (int ) : m_i (i) {}
void add (int howmuch) { m_i += howmuch; }
private:
int _i;
};
void reset (Trivial *triv)
{
memset (triv, 0, sizeof (Trivial));
}
void recompute (Trivial *triv)
{
reset (triv); // start over
triv->add (10); // whoops, null reference.
}
It's also warn for memset of trivial types that aren't standard
layout due to having a mix of public/protected/private fields,
which is likely not a real problem in practice, but I'd call
those a code smell that warrants a warning too:
struct S
{
private:
int a;
public:
int b;
};
S s;
memset (, 0, sizeof (S));
Playing with the patch, I noticed that you can't silence
it by casting the pointer to void, but you can with
casting to char, like:
void copy (B *dst, B *src)
{
memcpy (dst, src, sizeof (*src)); // warns
memcpy ((void*) dst, (void *) src, sizeof (*src)); // still warns
memcpy ((char*) dst, (char *) src, sizeof (*src)); // doesn't warn
memcpy ((unsigned char*) dst, (unsigned char *) src, sizeof (*src)); //
doesn't warn
}
I can understand how we end up like that, given char's magic properties, but
still
I think many will reach for "void" first if they (really really) need to add a
cast to
silence the warning. In any case, I think it'd be very nice to add cases with
such
casts to the new tests, and maybe mention it in the documentation too.
Thanks,
Pedro Alves
Re: [PATCH 3/8] Simplify representation of locations of a block.
On 04/28/2017 08:01 PM, Pedro Alves wrote:
> On 04/28/2017 05:28 PM, Martin Sebor wrote:
>> On 04/28/2017 05:47 AM, Nathan Sidwell wrote:
>
>>>> @@ -427,9 +429,31 @@ static void output_lines (FILE *, const source_t
>>>> *);
>>>> static char *make_gcov_file_name (const char *, const char *);
>>>> static char *mangle_name (const char *, char *);
>>>> static void release_structures (void);
>>>> -static void release_function (function_t *);
>>>> extern int main (int, char **);
>>>> +function_info::function_info ()
>>>> +{
>>>> + memset (this, 0, sizeof (*this));
>>>
>>> EW. ok with a comment about function_info's c++11's PoDness.
>>
>> Unless it's some other kind of vector, the patch adds a vector
>> member to the class, which makes it not a PoD.(*)
>
> Funny, just this week we added this to gdb to catch such misuses
> at compile time:
> https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
> https://sourceware.org/ml/gdb-patches/2017-04/msg00381.html
I think that teaching g++ new warnings for these issues would
be neat, BTW.
Thanks,
Pedro Alves
Re: [PATCH 3/8] Simplify representation of locations of a block.
On 04/28/2017 05:28 PM, Martin Sebor wrote:
> On 04/28/2017 05:47 AM, Nathan Sidwell wrote:
>>> @@ -427,9 +429,31 @@ static void output_lines (FILE *, const source_t
>>> *);
>>> static char *make_gcov_file_name (const char *, const char *);
>>> static char *mangle_name (const char *, char *);
>>> static void release_structures (void);
>>> -static void release_function (function_t *);
>>> extern int main (int, char **);
>>> +function_info::function_info ()
>>> +{
>>> + memset (this, 0, sizeof (*this));
>>
>> EW. ok with a comment about function_info's c++11's PoDness.
>
> Unless it's some other kind of vector, the patch adds a vector
> member to the class, which makes it not a PoD.(*)
Funny, just this week we added this to gdb to catch such misuses
at compile time:
https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
https://sourceware.org/ml/gdb-patches/2017-04/msg00381.html
Thanks,
Pedro Alves
