Re: Updated Sourceware infrastructure plans
On 2024-05-01 22:04, Simon Marchi wrote: > The Change-Id trailer works very well for Gerrit: once you have the hook > installed you basically never have to think about it again, and Gerrit > is able to track patch versions perfectly accurately. A while ago, I > asked patchwork developers if they would be open to support something > like that to track patches, and they said they wouldn't be against it > (provided it's not mandatory) [1]. But somebody would have to implement > it. > > Simon > > [1] https://github.com/getpatchwork/patchwork/issues/327 +1000. It's mind boggling to me that people would accept Gerrit, which means that they'd accept Change-Id:, but then they wouldn't accept Change-Id: with a different system... :-)
Re: Updated Sourceware infrastructure plans
On 2024-05-01 22:26, Mark Wielaard wrote: > For now I am cleaning up Sergio's gerrit setup and upgrading it to the > latest version, so people can at least try it out. Although I must > admit that I seem to be the only Sourcewware PLC member that believes > this is very useful use of our resources. Even the biggest proponents > of gerrit seem to believe no project will actually adopt it. And on > irc there were some people really critical of the effort. It seems you > either love or really hate gerrit... When GDB upstream tried to use gerrit, I found it basically impossible to follow development, given the volume... The great thing with email is the threading of discussions. A discussion can fork into its own subthread, and any sane email client will display the discussion tree. Email archives also let you follow the discussion subthreads. That is great for archaeology too. With Gerrit that was basically lost, everything is super flat. And then following development via the gerrit instance website alone is just basically impossible too. I mean, gerrit is great to track your own patches, and for the actual review and diffing between versions. But for a maintainer who wants to stay on top of a project, then it's severely lacking, IME and IMO. (Note: I've been using Gerrit for a few years at AMD internally.)
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: More C type errors by default for GCC 14
On 2023-05-12 7:01 a.m., Po Lu via Gcc wrote: > Jason Merrill writes: > >> You shouldn't have to change any of those, just configure with CC="gcc >> -fwhatever". > > If it were so simple... > > Many Makefiles come with a habit of using not CC_FOR_BUILD, but just > `cc', to build programs which are run on the build machine. Then write a wrapper script named "cc", and put that in the PATH: $ cat cc #!/bin/sh exec /my/real/gcc -fwhatever "$@" Done.
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: [RFC] Using std::unique_ptr and std::make_unique in our code
On 2022-07-12 11:45 a.m., Jonathan Wakely wrote:
> On Tue, 12 Jul 2022 at 11:22, Florian Weimer via Gcc wrote:
>>
>> * Pedro Alves:
>>
>>> For example, for the type above, we'd have:
>>>
>>> typedef std::unique_ptr pending_diagnostic_up;
>>>
>>> and then:
>>>
>>> -pending_diagnostic *d,
>>> +pending_diagnostic_up d,
>>>
>>> I would suggest GCC have a similar guideline, before people start
>>> using foo_ptr, bar_unp, quux_p, whatnot diverging styles.
>>
>> This doesn't seem to provide much benefit over writing
>>
>> uP d;
>>
>> and with that construct, you don't need to worry about the actual
>> relationship between pending_diagnostic and pending_diagnostic_up.
>>
>> I think the GDB situation is different because many of the types do not
>> have proper destructors, so std::unique_ptr needs a custom deleter.
>
>
> A fairly common idiom is for the type to define the typedef itself:
>
> struct pending_diagnostic {
> using ptr = std::unique_ptr;
> // ...
> };
>
> Then you use pending_diagnostic::ptr. If you want a custom deleter for
> the type, you add it to the typedef.
>
> Use a more descriptive name like uptr or uniq_ptr instead of "ptr" if
> you prefer.
>
Only works if you can change the type, though. Sometimes you can't,
as it comes from a library.
Re: [RFC] Using std::unique_ptr and std::make_unique in our code
On 2022-07-12 11:21 a.m., Florian Weimer wrote: > * Pedro Alves: > >> For example, for the type above, we'd have: >> >> typedef std::unique_ptr pending_diagnostic_up; >> >> and then: >> >> - pending_diagnostic *d, >> + pending_diagnostic_up d, >> >> I would suggest GCC have a similar guideline, before people start >> using foo_ptr, bar_unp, quux_p, whatnot diverging styles. > > This doesn't seem to provide much benefit over writing > > uP d; > > and with that construct, you don't need to worry about the actual > relationship between pending_diagnostic and pending_diagnostic_up. Given the guideline, nobody ever worries about that. When you see "_up", you just know it's a unique pointer. And as you point out, there's the custom deleters case to consider too. > > I think the GDB situation is different because many of the types do not > have proper destructors, so std::unique_ptr needs a custom deleter. Yes, there are a few cases like but it's not "many" as you suggest, and most are types for which we have no control, like 3rd party library types, like debuginfod, curses, python. Most of the rest of the custom deleter cases are instead because of intrusive refcounting. I.e., the deleter decrements the object's refcount, instead of deleting the object straight away. These are valid cases, not "GDB is doing it wrong, so GCC won't have to bother". I would suspect that GCC will end up with a good number of custom deleters as well.
Re: [RFC] Using std::unique_ptr and std::make_unique in our code
Hi! On 2022-07-08 9:46 p.m., David Malcolm via Gcc wrote: > - pending_diagnostic *d, > + std::unique_ptr d, I see that you didn't add any typedef for std::unique_ptr in this patch. It will be inevitable that people will start adding them, for conciseness, IME, though. To avoid diverging naming styles for such typedefs in the codebase, GDB settled on using the "_up" suffix (for Unique Pointer) quite early in the C++11 conversion, and we use such typedefs pervasively nowadays. For example, for the type above, we'd have: typedef std::unique_ptr pending_diagnostic_up; and then: - pending_diagnostic *d, + pending_diagnostic_up d, I would suggest GCC have a similar guideline, before people start using foo_ptr, bar_unp, quux_p, whatnot diverging styles. And it would be nice if GCC followed the same nomenclature style as GDB, so we could have one single guideline for the whole GNU toolchain, so people moving between codebases only had to learn one guideline. Pedro Alves
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.
GSoC, Make cp-demangle non-recursive and async-signal safety
Hi! I noticed the discussions about making cp-demangle use malloc/free instead of recursion, and I wonder about signal handlers, and I don't see that mentioned in https://gcc.gnu.org/wiki/SummerOfCode's description of the project. See my question to Ian a few years back, here, and his answer: https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg00696.html ~~~ Ian says: > Pedro says: > 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. ~~~ Grepping the gcc tree, I see that libsanitizer uses those entry points. Is async-signal safety no longer a consideration/concern? Or will those entry points continue to work without calling malloc/free somehow?
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: GCC selftest improvements
On 10/29/19 8:40 AM, Richard Biener wrote: > On Mon, Oct 28, 2019 at 10:47 PM Jakub Jelinek wrote: >> >> As discussed earlier, we gain most through C++11 support, there is no need >> to jump to C++17 or C++20 as requirement. > > Yes, I've agreed to raise the requirement to GCC 4.8 which provides > C++11 support. > > For convenience we could also provide a configure-time hint if the host > compiler > doesn't have C++11 support or is older than 4.8.2 (I think .1 has some > issues). > Rather than only running into some obscure errors later on. FWIW, GDB uses a slightly modified AX_CXX_COMPILE_STDCXX to check for C++11 support at configure time, and add -std=gnu++11 if the necessary, adding nothing if the compiler supports C++11 or later OOTB (so that you can still access C++14-or-later features conditionally). https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/ax_cxx_compile_stdcxx.m4 https://www.gnu.org/software/autoconf-archive/ax_cxx_compile_stdcxx.html https://sourceware.org/ml/gdb-patches/2016-10/msg00775.html In practice, that returns "supports" for GCC 4.8 and above, which is GDB's minimum requirement. I'm not sure about 4.8.x patch level. Thanks, Pedro Alves
Re: GCC selftest improvements
On 10/26/19 11:46 PM, Eric Gallager wrote: > Nicholas Krause was also wanting to move to C++11 recently: > https://gcc.gnu.org/ml/gcc/2019-10/msg00110.html (this month) > https://gcc.gnu.org/ml/gcc/2019-09/msg00228.html (last month) > As I said in that thread, I'd want to try just toggling -Wnarrowing > from off to on first before going full C++11. Why? GDB went the other way when it moved to C++11. It switched to C++11, and for several months, used -Wno-narrowing to quiet the thousands of warnings. https://gcc.gnu.org/wiki/FAQ#Wnarrowing > So, GCC 10 would be > C++98 + -Wnarrowing, and then GCC 11 could be full C++11. Plus then > the GCC version numbers would also line up with the version of C++ > being used. Thanks, 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: Moving to C++11
On 9/26/19 9:08 AM, Richard Biener wrote: > Note the main issue is host compiler support. I'm not sure if C++11 would > be the step we'd gain most - for some hashtable issues I'd have needed > std::move support for example. There's always the possibility to > require an intermediate step (first build GCC 5, with that you can build > trunk, etc.), a install.texi clarification could be useful here (or even > some automation via a contrib/ script). > > I'm not too worried about requiring even a C++14 compiler, for the > set of products we still release latest compilers we have newer > GCCs available we can use for building them (even if those are > not our primary supported compilers which would limit us to > GCC 4.8). FWIW, GDB requires C++11 nowadays, and the baseline required GCC version is GCC 4.8.1. The current policy is here: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#When_is_GDB_going_to_start_requiring_C.2B-.2B-NN_.3F Pasted for convenience: When is GDB going to start requiring C++NN ? Our general policy is to wait until the oldest compiler that supports C++NN is at least 3 years old. Rationale: We want to ensure reasonably widespread compiler availability, to lower barrier of entry to GDB contributions, and to make it easy for users to easily build new GDB on currently supported stable distributions themselves. 3 years should be sufficient for latest stable releases of distributions to include a compiler for the standard, and/or for new compilers to appear as easily installable optional packages. Requiring everyone to build a compiler first before building GDB, which would happen if we required a too-new compiler, would cause too much inconvenience. That was decided 3 years ago, so I guess we'd be good for a reevaluation, though I don't particularly miss C++14 features all that much, so I wouldn't mind staying with C++11 for a while longer in GDB. But if GCC jumps to C++14, I think GDB would follow along. Just FYI. C++03 -> C++11 makes a great difference. Particularly std::move and rvalue references are a game changer. 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: [RFC] Update coding conventions to restrict use of non-const references
On 07/12/2018 05:17 PM, Richard Sandiford wrote:
> Pedro Alves writes:
>> (an
>>> alternative to pointers is to return a struct with the wide int result
>>> and the overflow flag),
>>
>> +1. I've been pushing GDB in that direction whenever possible.
>
> I agree that can sometimes be better. I guess it depends on the
> context. If a function returns a bool and some other data that has no
> obvious "failure" value, it can be easier to write chained conditions if
> the function returns the bool and provides the other data via an output
> parameter.
I agree it depends on context, though your example sounds like a
case for std::optional. (We have gdb::optional in gdb, since
std::optional is C++17 and gdb requires C++11. LLVM has a similar
type, I believe.)
>
>>> but ...
>>>
>>>> +int *elt = array[i]; // OK
>>>> +int elt = array[i]; // Please avoid
>>>
>>> ... this seems unnecessary. If the function is so long that the fact
>>> elt is a reference can easily get lost, the problem is the length of
>>> the function, not the use of a reference.
>>>
>>
>> +1000. This seems really unnecessary. References have the advantage
>> of implicit never-null semantics, for instance.
>
> The nonnullness is there either way in the above example though.
>
> I don't feel as strongly about the non-const-reference variables, but for:
>
> int = array[i];
> ...
> elt = 1;
>
> it can be easy to misread that "elt = 1" is changing more than just
> a local variable.
I think that might be just the case for people who haven't used
references before, and once you get some exposure, the effect
goes away. See examples below.
>
> I take Jonathan's point that it shouldn't be much of a problem if
> functions are a reasonable size, but we've not tended to be very
> good at keeping function sizes down. I guess part of the problem
> is that even functions that start off small tend to grow over time.
>
> We have been better at enforcing more specific rules like the ones
> in the patch. And that's easier to do because a patch either adds
> references or it doesn't. It's harder to force (or remember to force)
> someone to split up a function if they're adding 5 lines to one that is
> already 20 lines long. Then for the next person it's 25 lines long, etc.
>
> Also, the "int *elt" approach copes with cases in which "elt" might
> have to be redirected later, so we're going to need to use it in some
> cases.
Sure, if you need to reseat, certainly use a pointer. That actually
highlights an advantage of references -- the fact that you are sure
nothing could reseat it. With a local pointer, you need to be mindful
of that. "Does this loop change the pointer?" Etc. If the semantics
of the function call for having a variable that is not meant to be
reseated, why not allow expressing it with a reference.
"Write what you mean." Of course, just like all code, there's going
to be a judgment call.
A place where references frequently appear is in C++11 range-for loops.
Like, random example from gdb's codebase:
for (const symbol_search : symbols)
GCC doesn't yet require C++11, but it should be a matter
of time, one would hope.
Other places references appear often is in coming up with
an alias to avoid repeating long expressions, like
for example (again from gdb):
auto = objfile->per_bfd->demangled_hash_languages;
auto it = std::lower_bound (vec.begin (), vec.end (),
MSYMBOL_LANGUAGE (sym));
if (it == vec.end () || *it != MSYMBOL_LANGUAGE (sym))
vec.insert (it, MSYMBOL_LANGUAGE (sym));
I don't think the fact that "vec" is a reference here confuses
anyone.
That was literally a random sample (grepped for "&[a-z]* = ") so
I ended up picking one with C++11 "auto", but here's another
random example, spelling out the type name, similarly using
a reference as a shorthand alias:
osdata_item = osdata->items.back ();
item.columns.emplace_back (std::move (data->property_name),
std::string (body_text));
> It's then a question of whether "int " is useful enough that
> it's worth accepting it too, and having a mixture of styles in the codebase.
I don't see it as a mixture of styles, as I don't see
pointers and references the exact same thing, but rather
see references as another tool in the box.
Thanks,
Pedro Alves
Re: [RFC] Update coding conventions to restrict use of non-const references
On 07/12/2018 12:40 PM, Jonathan Wakely wrote: > On Thu, 12 Jul 2018 at 11:41, Richard Sandiford wrote: >> +Only use non-constant references in the following situations: >> + >> + >> + >> +when they are necessary to conform to a standard interface, such as >> +the first argument to a non-member operator+= > > And the return value of such operators (which also applies to member > operators, which is the more conventional way to write compound > assignment operators). > >> +in a return value, when providing access to something that is known >> +to be nonnull >> + >> + >> + >> +In other situations the convention is to use pointers instead. >> + >> + >> + >> +HOST_WIDE_INT do_arith (..., bool *overflow); // OK >> +HOST_WIDE_INT do_arith (..., bool overflow); // Please avoid > > I understand the objection to using references for out parameters FWIW, GDB's conventions (which import GCC's coding conventions) already suggest avoiding non-const reference output parameters, so this won't affect us. But, FWIW, personally, while I used to be all for avoiding non-const reference output parameters, I no longer am, at least so strongly, nowadays. The reason being that the visual distinction can be easily lost with pointers too anyway: // the usual argument is that using pointers for output parameters shows // clearly that bar is going to be modified: function (foo, ); // but well, we often works with pointers, and if "bar" is a pointer, // we don't get any visual clue anyway either: function (foo, bar); // which suggests that what really helps is seeing the output // variable nearby, suggesting to define it right before the // function call that fills it in, and I would go as far // as saying that clearer symbol names help even more. For e.g.: B bar_out; fill_bar (foo, bar_out); (an > alternative to pointers is to return a struct with the wide int result > and the overflow flag), +1. I've been pushing GDB in that direction whenever possible. > but ... > >> +int *elt = array[i]; // OK >> +int elt = array[i]; // Please avoid > > ... this seems unnecessary. If the function is so long that the fact > elt is a reference can easily get lost, the problem is the length of > the function, not the use of a reference. > +1000. This seems really unnecessary. References have the advantage of implicit never-null semantics, for instance. 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: -Wclass-memaccess warning should be in -Wextra, not -Wall
On 07/06/2018 12:14 AM, Soul Studios wrote: > Having benchmarked the alternatives memcpy/memmove/memset definitely makes a > difference in various scenarios. That sounds like a missing optimization in the compiler. If you have valid testcases, I think it would be a good idea to file them in bugzilla. 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: [PING][PATCH] gdb/x86: Fix `-Wstrict-overflow' build error in `i387_collect_xsave'
On 05/22/2018 12:14 PM, Maciej W. Rozycki wrote: > On Tue, 15 May 2018, Maciej W. Rozycki wrote: > >> gdb/ >> * i387-tdep.c (i387_collect_xsave): Make `i' unsigned. > > Ping for: <https://patchwork.sourceware.org/patch/27269/>. OK. 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: Using gnu::unique_ptr to avoid manual cleanups (was Re: [PATCH 2/2] use unique_ptr some)
On 10/17/2017 03:57 PM, David Malcolm wrote:
> Given that we build with -fno-exceptions, what are we guaranteed about
> what happens when "new" fails? (am I right in thinking that a failed
> allocation returns NULL in this case?). Is XNEWVEC preferable here?
No, that's incorrect. Even with -fno-exceptions, if new fails,
then an exception is thrown. And then because the unwinder
doesn't find a frame that handles the exception, std::terminate
is called...
You can easily see it with this:
$ cat new-op.cc
int main ()
{
char * p = new char [-1];
return 0;
}
$ g++ new-op.cc -o new-op -g3 -O0 -fno-exceptions
$ ./new-op
terminate called after throwing an instance of 'std::bad_alloc'
what(): std::bad_alloc
Aborted (core dumped)
If you want new to return NULL on allocation failure, you either
have to call the std::nothrow_t overload of new:
char* p = new (std::nothrow) char [-1];
or you have to replace [1] the global operator new to not throw.
>
> i.e. do we prefer:
>
> (A)
>
> gnu::unique_ptr<uint32_t[]> data (new uint32_t[size + 1]);
>
> (implicitly delete-ing the data, with an access-through-NULL if the
> allocation fails)
That'd be my preference too [2].
Though I think that GCC should replace the global
operator new/new[] functions to call xmalloc instead of malloc.
Like:
void *
operator new (std::size_t sz)
{
return xmalloc (sz);
}
void *
operator new (std::size_t sz, const std::nothrow_t&)
{
/* malloc (0) is unpredictable; avoid it. */
if (sz == 0)
sz = 1;
return malloc (sz);
}
Then memory allocated by all new expressions, including those done
via the standard allocators within standard containers, std::vector,
std::string, etc., ends up in xmalloc, which calls xmalloc_failed on
allocation failure. I.e., replace operator new to behave just
like xmalloc instead of letting it throw an exception that is
guaranteed to bring down gcc, badly.
Note there are already many "new" expressions/calls in GCC
today. Any of those can bring down GCC.
This is very much like what I did for GDB. See GDB's (latest)
version here:
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/common/new-op.c;hb=bbe910e6e1140cb484a74911f3cea854cf9e7e2a
GDB's version still throws, because well, GDB uses exceptions.
[1] Replacing global operator new is something that is totally
defined by the standard (unlike -fno-exceptions).
See "global replacements"
at <http://en.cppreference.com/w/cpp/memory/new/operator_new>.
[2] - Or in many cases, use a std::vector instead. And if
you care about not value-initializing elements, see gdb's
gdb/common/{def-vector, default-init-alloc}.h.
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: Slides from GNU Tools Cauldron
On 10/04/2017 08:09 PM, Jan Hubicka wrote: > Hello, > all but one videos from this year Cauldron has been edited and are now linked > from https://gcc.gnu.org/wiki/cauldron2017 (plugins BoF will appear till end > of week). > > I would also like to update the page with links to slides. If someone beats > me > on this and adds some or all of them as attachements to the page, I would be > very happy :) I like the table with direct links to talks/slides/videos on last year's page, so I spent a while this morning adding a table to this year's page. Hope others find that useful too. 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: gcc behavior on memory exhaustion
On 08/10/2017 10:22 PM, Florian Weimer wrote: > * Andrew Haley: > >> On 09/08/17 14:05, Andrew Roberts wrote: >>> 2) It would be nice to see some sort of out of memory error, rather than >>> just an ICE. >> >> There's nothing we can do: the kernel killed us. We can't emit any >> message before we die. (killed) tells you that we were killed, but >> we don't know who done it. > > The driver already prints a message. > > The siginfo_t information should indicate that the signal originated > from the kernel. OOC, where? While a parent process can use "waitid" to get a siginfo_t with information about the child exit, that siginfo_t is not the same siginfo_t a signal handler would get as argument if you could catch/intercept SIGKILL, which you can't on Linux. I.e., checking for e.g., si_code == SI_KERNEL in the siginfo filled in by waitid won't work, because that siginfo_t has si_code values for SIGCHLD [CLD_EXITED/CLD_KILLED/etc.], not for the signal that actually killed the process. Doesn't seem to give you any more useful information beyond the what you can already get using waitpid (which is what libiberty's pex code in question uses) and WIFSIGNALED/WTERMSIG. > It seems that for SIGKILL, there are currently three > causes in the kernel: the OOM killer, some apparently unreachable code > in ptrace, and something cgroups-related. The latter would likely > take down the driver process, too, so a kernel-originated SIGKILL > strongly points to the OOM killer. > > But the kernel could definitely do better and set a flag for SIGKILL. Meanwhile, maybe just having the driver check for SIGKILL and enumerate likely causes would be better than the status quo. 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
