* [analyzer PATCH] Restore bootstrap with g++ 4.8. @ 2024-06-07 18:40 Roger Sayle 2024-06-07 20:18 ` David Malcolm 2024-06-14 9:36 ` Jonathan Wakely 0 siblings, 2 replies; 9+ messages in thread From: Roger Sayle @ 2024-06-07 18:40 UTC (permalink / raw) To: gcc-patches; +Cc: 'David Malcolm' [-- Attachment #1: Type: text/plain, Size: 1192 bytes --] This patch restores bootstrap when using g++ 4.8 as a host compiler. Returning a std::unique_ptr requires a std::move on C++ compilers (pre-C++17) that don't guarantee copy elision/return value optimization. Bootstrapped on x86_64-pc-linux-gnu using both gcc 4.8.5 (system) and gcc 10.2.1 (using "scl enable devetoolset-10") as host compilers. Ok for mainline? 2024-06-07 Roger Sayle <roger@nextmovesoftware.com> gcc/analyzer/ChangeLog * constraint-manager.cc (equiv_class::make_dump_widget): Use std::move to return a std::unique_ptr. (bounded_ranges_constraint::make_dump_widget): Likewise. (constraint_manager::make_dump_widget): Likewise. * program_state.cc (sm_state_map::make_dump_widget): Likewise. (program_state::make_dump_widget): Likewise. * region-model.cc (region_to_value_map::make_dump_widget): Likewise. (region_model::make_dump_widget): Likewise. * region.cc (region::make_dump_widget): Likewise. * store.cc (binding_cluster::make_dump_widget): Likewise. (store::make_dump_widget): Likewise. * svalue.cc (svalue::make_dump_widget): Likewise. Thanks in advance, Roger -- [-- Attachment #2: patchup.txt --] [-- Type: text/plain, Size: 4237 bytes --] diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc index 707385d..883f33b 100644 --- a/gcc/analyzer/constraint-manager.cc +++ b/gcc/analyzer/constraint-manager.cc @@ -1176,7 +1176,7 @@ equiv_class::make_dump_widget (const text_art::dump_widget_info &dwi, ec_widget->add_child (tree_widget::make (dwi, &pp)); } - return ec_widget; + return std::move (ec_widget); } /* Generate a hash value for this equiv_class. @@ -1500,7 +1500,7 @@ make_dump_widget (const text_art::dump_widget_info &dwi) const (tree_widget::from_fmt (dwi, nullptr, "ec%i bounded ranges", m_ec_id.as_int ())); m_ranges->add_to_dump_widget (*brc_widget.get (), dwi); - return brc_widget; + return std::move (brc_widget); } bool @@ -1853,7 +1853,7 @@ constraint_manager::make_dump_widget (const text_art::dump_widget_info &dwi) con if (cm_widget->get_num_children () == 0) return nullptr; - return cm_widget; + return std::move (cm_widget); } /* Attempt to add the constraint LHS OP RHS to this constraint_manager. diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index dc2d4bd..efaf569 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -382,7 +382,7 @@ sm_state_map::make_dump_widget (const text_art::dump_widget_info &dwi, state_widget->add_child (tree_widget::make (dwi, pp)); } - return state_widget; + return std::move (state_widget); } /* Return true if no states have been set within this map @@ -1247,7 +1247,7 @@ program_state::make_dump_widget (const text_art::dump_widget_info &dwi) const state_widget->add_child (smap->make_dump_widget (dwi, m_region_model)); } - return state_widget; + return std::move (state_widget); } /* Update this program_state to reflect a top-level call to FUN. diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index d142d85..4fbc970 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -288,7 +288,7 @@ make_dump_widget (const text_art::dump_widget_info &dwi) const sval->dump_to_pp (pp, true); w->add_child (text_art::tree_widget::make (dwi, pp)); } - return w; + return std::move (w); } /* Attempt to merge THIS with OTHER, writing the result @@ -556,7 +556,7 @@ region_model::make_dump_widget (const text_art::dump_widget_info &dwi) const m_mgr->get_store_manager ())); model_widget->add_child (m_constraints->make_dump_widget (dwi)); model_widget->add_child (m_dynamic_extents.make_dump_widget (dwi)); - return model_widget; + return std::move (model_widget); } /* Assert that this object is valid. */ diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc index 71bae97..050feb6 100644 --- a/gcc/analyzer/region.cc +++ b/gcc/analyzer/region.cc @@ -1119,7 +1119,7 @@ region::make_dump_widget (const text_art::dump_widget_info &dwi, if (m_parent) w->add_child (m_parent->make_dump_widget (dwi, "parent")); - return w; + return std::move (w); } void diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index d14cfa3..b20bc29 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -1489,7 +1489,7 @@ binding_cluster::make_dump_widget (const text_art::dump_widget_info &dwi, m_map.add_to_tree_widget (*cluster_widget, dwi); - return cluster_widget; + return std::move (cluster_widget); } } @@ -2766,7 +2766,7 @@ store::make_dump_widget (const text_art::dump_widget_info &dwi, store_widget->add_child (std::move (parent_reg_widget)); } - return store_widget; + return std::move (store_widget); } /* Get any svalue bound to REG, or NULL. */ diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index f1fd21e..b67780a 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -252,7 +252,7 @@ svalue::make_dump_widget (const text_art::dump_widget_info &dwi, add_dump_widget_children (*w, dwi); - return w; + return std::move (w); } /* If this svalue is a constant_svalue, return the underlying tree constant. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [analyzer PATCH] Restore bootstrap with g++ 4.8. 2024-06-07 18:40 [analyzer PATCH] Restore bootstrap with g++ 4.8 Roger Sayle @ 2024-06-07 20:18 ` David Malcolm 2024-06-14 9:36 ` Jonathan Wakely 1 sibling, 0 replies; 9+ messages in thread From: David Malcolm @ 2024-06-07 20:18 UTC (permalink / raw) To: Roger Sayle, gcc-patches On Fri, 2024-06-07 at 19:40 +0100, Roger Sayle wrote: > > This patch restores bootstrap when using g++ 4.8 as a host compiler. > Returning a std::unique_ptr requires a std::move on C++ compilers > (pre-C++17) that don't guarantee copy elision/return value > optimization. > > Bootstrapped on x86_64-pc-linux-gnu using both gcc 4.8.5 (system) and > gcc 10.2.1 (using "scl enable devetoolset-10") as host compilers. > Ok for mainline? Yes, thanks. Sorry for the breakage. Dave > > > 2024-06-07 Roger Sayle <roger@nextmovesoftware.com> > > gcc/analyzer/ChangeLog > * constraint-manager.cc (equiv_class::make_dump_widget): Use > std::move to return a std::unique_ptr. > (bounded_ranges_constraint::make_dump_widget): Likewise. > (constraint_manager::make_dump_widget): Likewise. > * program_state.cc (sm_state_map::make_dump_widget): > Likewise. > (program_state::make_dump_widget): Likewise. > * region-model.cc (region_to_value_map::make_dump_widget): > Likewise. > (region_model::make_dump_widget): Likewise. > * region.cc (region::make_dump_widget): Likewise. > * store.cc (binding_cluster::make_dump_widget): Likewise. > (store::make_dump_widget): Likewise. > * svalue.cc (svalue::make_dump_widget): Likewise. > > Thanks in advance, > Roger > -- > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [analyzer PATCH] Restore bootstrap with g++ 4.8. 2024-06-07 18:40 [analyzer PATCH] Restore bootstrap with g++ 4.8 Roger Sayle 2024-06-07 20:18 ` David Malcolm @ 2024-06-14 9:36 ` Jonathan Wakely 2024-06-14 10:26 ` Jonathan Wakely 1 sibling, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-06-14 9:36 UTC (permalink / raw) To: Roger Sayle; +Cc: gcc-patches, 'David Malcolm' On 07/06/24 19:40 +0100, Roger Sayle wrote: > >This patch restores bootstrap when using g++ 4.8 as a host compiler. >Returning a std::unique_ptr requires a std::move on C++ compilers >(pre-C++17) that don't guarantee copy elision/return value optimization. It doesn't though. The C++17 guaranteed copy elision rules are not relevant here. This is about lookup for the constructor used in the return statement, and whether that lookup considers the variable to be an lvalue or an rvalue. C++11 already says this is valid: i#include <memory> std::unique_ptr<int> f() { std::unique_ptr<int> m; return m; } See C++11 12.8 [class.copy] p31: This elision of copy/move operations, called copy elision, is permitted in the following circumstances (which may be combined to eliminate multiple copies): - in a return statement in a function with a class return type, when the expression is the name of a non-volatile automatic object (other than a function or catch-clause parameter) with the same cv- unqualified type as the function return type, the copy/move operation can be omitted by constructing the automatic object directly into the function’s return value and then p32: When the criteria for elision of a copy operation are met or would be met save for the fact that the source object is a function parameter, and the object to be copied is designated by an lvalue, overload resolution to select the constructor for the copy is first performed as if the object were designated by an rvalue. The constructor isn't required to be elided in C++11, but the compiler is required to use a move constructor instead of a copy constructor, if a move constructor is available. So you don't need to use std::move on the return value. And the code above compiles fine with gcc 4.8.5 on CentOS 7 (and even with 4.7.1). So the std::move calls you've added are redundant, and will cause -Wredundant-move warnings. What's the error you were seeing? >Bootstrapped on x86_64-pc-linux-gnu using both gcc 4.8.5 (system) and >gcc 10.2.1 (using "scl enable devetoolset-10") as host compilers. >Ok for mainline? > > >2024-06-07 Roger Sayle <roger@nextmovesoftware.com> > >gcc/analyzer/ChangeLog > * constraint-manager.cc (equiv_class::make_dump_widget): Use > std::move to return a std::unique_ptr. > (bounded_ranges_constraint::make_dump_widget): Likewise. > (constraint_manager::make_dump_widget): Likewise. > * program_state.cc (sm_state_map::make_dump_widget): Likewise. > (program_state::make_dump_widget): Likewise. > * region-model.cc (region_to_value_map::make_dump_widget): Likewise. > (region_model::make_dump_widget): Likewise. > * region.cc (region::make_dump_widget): Likewise. > * store.cc (binding_cluster::make_dump_widget): Likewise. > (store::make_dump_widget): Likewise. > * svalue.cc (svalue::make_dump_widget): Likewise. > >Thanks in advance, >Roger >-- > >diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc >index 707385d..883f33b 100644 >--- a/gcc/analyzer/constraint-manager.cc >+++ b/gcc/analyzer/constraint-manager.cc >@@ -1176,7 +1176,7 @@ equiv_class::make_dump_widget (const text_art::dump_widget_info &dwi, > ec_widget->add_child (tree_widget::make (dwi, &pp)); > } > >- return ec_widget; >+ return std::move (ec_widget); > } > > /* Generate a hash value for this equiv_class. >@@ -1500,7 +1500,7 @@ make_dump_widget (const text_art::dump_widget_info &dwi) const > (tree_widget::from_fmt (dwi, nullptr, > "ec%i bounded ranges", m_ec_id.as_int ())); > m_ranges->add_to_dump_widget (*brc_widget.get (), dwi); >- return brc_widget; >+ return std::move (brc_widget); > } > > bool >@@ -1853,7 +1853,7 @@ constraint_manager::make_dump_widget (const text_art::dump_widget_info &dwi) con > if (cm_widget->get_num_children () == 0) > return nullptr; > >- return cm_widget; >+ return std::move (cm_widget); > } > > /* Attempt to add the constraint LHS OP RHS to this constraint_manager. >diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc >index dc2d4bd..efaf569 100644 >--- a/gcc/analyzer/program-state.cc >+++ b/gcc/analyzer/program-state.cc >@@ -382,7 +382,7 @@ sm_state_map::make_dump_widget (const text_art::dump_widget_info &dwi, > state_widget->add_child (tree_widget::make (dwi, pp)); > } > >- return state_widget; >+ return std::move (state_widget); > } > > /* Return true if no states have been set within this map >@@ -1247,7 +1247,7 @@ program_state::make_dump_widget (const text_art::dump_widget_info &dwi) const > state_widget->add_child (smap->make_dump_widget (dwi, m_region_model)); > } > >- return state_widget; >+ return std::move (state_widget); > } > > /* Update this program_state to reflect a top-level call to FUN. >diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc >index d142d85..4fbc970 100644 >--- a/gcc/analyzer/region-model.cc >+++ b/gcc/analyzer/region-model.cc >@@ -288,7 +288,7 @@ make_dump_widget (const text_art::dump_widget_info &dwi) const > sval->dump_to_pp (pp, true); > w->add_child (text_art::tree_widget::make (dwi, pp)); > } >- return w; >+ return std::move (w); > } > > /* Attempt to merge THIS with OTHER, writing the result >@@ -556,7 +556,7 @@ region_model::make_dump_widget (const text_art::dump_widget_info &dwi) const > m_mgr->get_store_manager ())); > model_widget->add_child (m_constraints->make_dump_widget (dwi)); > model_widget->add_child (m_dynamic_extents.make_dump_widget (dwi)); >- return model_widget; >+ return std::move (model_widget); > } > > /* Assert that this object is valid. */ >diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc >index 71bae97..050feb6 100644 >--- a/gcc/analyzer/region.cc >+++ b/gcc/analyzer/region.cc >@@ -1119,7 +1119,7 @@ region::make_dump_widget (const text_art::dump_widget_info &dwi, > if (m_parent) > w->add_child (m_parent->make_dump_widget (dwi, "parent")); > >- return w; >+ return std::move (w); > } > > void >diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc >index d14cfa3..b20bc29 100644 >--- a/gcc/analyzer/store.cc >+++ b/gcc/analyzer/store.cc >@@ -1489,7 +1489,7 @@ binding_cluster::make_dump_widget (const text_art::dump_widget_info &dwi, > > m_map.add_to_tree_widget (*cluster_widget, dwi); > >- return cluster_widget; >+ return std::move (cluster_widget); > } > } > >@@ -2766,7 +2766,7 @@ store::make_dump_widget (const text_art::dump_widget_info &dwi, > store_widget->add_child (std::move (parent_reg_widget)); > } > >- return store_widget; >+ return std::move (store_widget); > } > > /* Get any svalue bound to REG, or NULL. */ >diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc >index f1fd21e..b67780a 100644 >--- a/gcc/analyzer/svalue.cc >+++ b/gcc/analyzer/svalue.cc >@@ -252,7 +252,7 @@ svalue::make_dump_widget (const text_art::dump_widget_info &dwi, > > add_dump_widget_children (*w, dwi); > >- return w; >+ return std::move (w); > } > > /* If this svalue is a constant_svalue, return the underlying tree constant. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [analyzer PATCH] Restore bootstrap with g++ 4.8. 2024-06-14 9:36 ` Jonathan Wakely @ 2024-06-14 10:26 ` Jonathan Wakely 2024-06-14 10:37 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-06-14 10:26 UTC (permalink / raw) To: Roger Sayle; +Cc: gcc-patches, 'David Malcolm' On 14/06/24 10:36 +0100, Jonathan Wakely wrote: >On 07/06/24 19:40 +0100, Roger Sayle wrote: >> >>This patch restores bootstrap when using g++ 4.8 as a host compiler. >>Returning a std::unique_ptr requires a std::move on C++ compilers >>(pre-C++17) that don't guarantee copy elision/return value optimization. > >It doesn't though. The C++17 guaranteed copy elision rules are not >relevant here. This is about lookup for the constructor used in the >return statement, and whether that lookup considers the variable to be >an lvalue or an rvalue. C++11 already says this is valid: > >i#include <memory> > >std::unique_ptr<int> f() >{ > std::unique_ptr<int> m; > return m; >} > >See C++11 12.8 [class.copy] p31: > > This elision of copy/move operations, called copy elision, is > permitted in the following circumstances (which may be combined to > eliminate multiple copies): > > - in a return statement in a function with a class return type, when > the expression is the name of a non-volatile automatic object (other > than a function or catch-clause parameter) with the same cv- > unqualified type as the function return type, the copy/move > operation can be omitted by constructing the automatic object > directly into the function’s return value > >and then p32: > > When the criteria for elision of a copy operation are met or would > be met save for the fact that the source object is a function > parameter, and the object to be copied is designated by an lvalue, > overload resolution to select the constructor for the copy is first > performed as if the object were designated by an rvalue. > >The constructor isn't required to be elided in C++11, but the compiler >is required to use a move constructor instead of a copy constructor, >if a move constructor is available. So you don't need to use std::move >on the return value. > >And the code above compiles fine with gcc 4.8.5 on CentOS 7 (and even >with 4.7.1). > >So the std::move calls you've added are redundant, and will cause >-Wredundant-move warnings. > >What's the error you were seeing? I can reproduce it: /home/test/src/gcc/gcc/analyzer/constraint-manager.cc: In member function ‘std::unique_ptr<text_art::widget> ana::equiv_class::make_dump_widget(const text_art::dump_widget_info&, unsigned int) const’: /home/test/src/gcc/gcc/analyzer/constraint-manager.cc:1179:10: error: cannot bind ‘std::unique_ptr<text_art::tree_widget>’ lvalue to ‘std::unique_ptr<text_art::tree_widget>&&’ return ec_widget; ^ Odd ... I'm looking into it ... ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [analyzer PATCH] Restore bootstrap with g++ 4.8. 2024-06-14 10:26 ` Jonathan Wakely @ 2024-06-14 10:37 ` Jonathan Wakely 2024-06-14 10:42 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-06-14 10:37 UTC (permalink / raw) To: Roger Sayle; +Cc: gcc-patches, 'David Malcolm' On 14/06/24 11:26 +0100, Jonathan Wakely wrote: >On 14/06/24 10:36 +0100, Jonathan Wakely wrote: >>On 07/06/24 19:40 +0100, Roger Sayle wrote: >>> >>>This patch restores bootstrap when using g++ 4.8 as a host compiler. >>>Returning a std::unique_ptr requires a std::move on C++ compilers >>>(pre-C++17) that don't guarantee copy elision/return value optimization. >> >>It doesn't though. The C++17 guaranteed copy elision rules are not >>relevant here. This is about lookup for the constructor used in the >>return statement, and whether that lookup considers the variable to be >>an lvalue or an rvalue. C++11 already says this is valid: >> >>i#include <memory> >> >>std::unique_ptr<int> f() >>{ >> std::unique_ptr<int> m; >> return m; >>} >> >>See C++11 12.8 [class.copy] p31: >> >> This elision of copy/move operations, called copy elision, is >> permitted in the following circumstances (which may be combined to >> eliminate multiple copies): >> >> - in a return statement in a function with a class return type, when >> the expression is the name of a non-volatile automatic object (other >> than a function or catch-clause parameter) with the same cv- >> unqualified type as the function return type, the copy/move >> operation can be omitted by constructing the automatic object >> directly into the function’s return value >> >>and then p32: >> >> When the criteria for elision of a copy operation are met or would >> be met save for the fact that the source object is a function >> parameter, and the object to be copied is designated by an lvalue, >> overload resolution to select the constructor for the copy is first >> performed as if the object were designated by an rvalue. >> >>The constructor isn't required to be elided in C++11, but the compiler >>is required to use a move constructor instead of a copy constructor, >>if a move constructor is available. So you don't need to use std::move >>on the return value. >> >>And the code above compiles fine with gcc 4.8.5 on CentOS 7 (and even >>with 4.7.1). >> >>So the std::move calls you've added are redundant, and will cause >>-Wredundant-move warnings. >> >>What's the error you were seeing? > >I can reproduce it: > >/home/test/src/gcc/gcc/analyzer/constraint-manager.cc: In member function ‘std::unique_ptr<text_art::widget> ana::equiv_class::make_dump_widget(const text_art::dump_widget_info&, unsigned int) const’: >/home/test/src/gcc/gcc/analyzer/constraint-manager.cc:1179:10: error: cannot bind ‘std::unique_ptr<text_art::tree_widget>’ lvalue to ‘std::unique_ptr<text_art::tree_widget>&&’ > return ec_widget; > ^ > >Odd ... I'm looking into it ... Ah, the problem here is that the function's return type is std::unique_ptr<text_art::widget> but the return value is std::unique_ptr<tree_widget> and that requires the converting constructor, not the move constructor. That *did* change after C++11, and isn't supported by G++ until 5.1 Is there a reason that function has to return unique_ptr<widget> instead of unique_ptr<tree_widget>? The conversion to ptr-to-base could happen at the call site (where you always have an rvalue) instead of on the return value. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [analyzer PATCH] Restore bootstrap with g++ 4.8. 2024-06-14 10:37 ` Jonathan Wakely @ 2024-06-14 10:42 ` Jonathan Wakely 2024-06-14 10:44 ` Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-06-14 10:42 UTC (permalink / raw) To: Roger Sayle; +Cc: gcc-patches, 'David Malcolm' On 14/06/24 11:37 +0100, Jonathan Wakely wrote: >On 14/06/24 11:26 +0100, Jonathan Wakely wrote: >>On 14/06/24 10:36 +0100, Jonathan Wakely wrote: >>>On 07/06/24 19:40 +0100, Roger Sayle wrote: >>>> >>>>This patch restores bootstrap when using g++ 4.8 as a host compiler. >>>>Returning a std::unique_ptr requires a std::move on C++ compilers >>>>(pre-C++17) that don't guarantee copy elision/return value optimization. >>> >>>It doesn't though. The C++17 guaranteed copy elision rules are not >>>relevant here. This is about lookup for the constructor used in the >>>return statement, and whether that lookup considers the variable to be >>>an lvalue or an rvalue. C++11 already says this is valid: >>> >>>i#include <memory> >>> >>>std::unique_ptr<int> f() >>>{ >>> std::unique_ptr<int> m; >>> return m; >>>} >>> >>>See C++11 12.8 [class.copy] p31: >>> >>>This elision of copy/move operations, called copy elision, is >>>permitted in the following circumstances (which may be combined to >>>eliminate multiple copies): >>> >>>- in a return statement in a function with a class return type, when >>>the expression is the name of a non-volatile automatic object (other >>>than a function or catch-clause parameter) with the same cv- >>>unqualified type as the function return type, the copy/move >>>operation can be omitted by constructing the automatic object >>>directly into the function’s return value >>> >>>and then p32: >>> >>>When the criteria for elision of a copy operation are met or would >>>be met save for the fact that the source object is a function >>>parameter, and the object to be copied is designated by an lvalue, >>>overload resolution to select the constructor for the copy is first >>>performed as if the object were designated by an rvalue. >>> >>>The constructor isn't required to be elided in C++11, but the compiler >>>is required to use a move constructor instead of a copy constructor, >>>if a move constructor is available. So you don't need to use std::move >>>on the return value. >>> >>>And the code above compiles fine with gcc 4.8.5 on CentOS 7 (and even >>>with 4.7.1). >>> >>>So the std::move calls you've added are redundant, and will cause >>>-Wredundant-move warnings. >>> >>>What's the error you were seeing? >> >>I can reproduce it: >> >>/home/test/src/gcc/gcc/analyzer/constraint-manager.cc: In member function ‘std::unique_ptr<text_art::widget> ana::equiv_class::make_dump_widget(const text_art::dump_widget_info&, unsigned int) const’: >>/home/test/src/gcc/gcc/analyzer/constraint-manager.cc:1179:10: error: cannot bind ‘std::unique_ptr<text_art::tree_widget>’ lvalue to ‘std::unique_ptr<text_art::tree_widget>&&’ >> return ec_widget; >> ^ >> >>Odd ... I'm looking into it ... > >Ah, the problem here is that the function's return type is >std::unique_ptr<text_art::widget> but the return value is >std::unique_ptr<tree_widget> and that requires the converting >constructor, not the move constructor. > >That *did* change after C++11, and isn't supported by G++ until 5.1 That changed with https://cplusplus.github.io/CWG/issues/1579.html which is in C++14 and is a DR against C++11, but of course compilers that implement the original C++11 rules don't support that DR. Arguably, g++ should not give -Wredundant-move warnings here when using the -std=c++11 dialect, because it's only redundant if the C++11 compiler implements that DR. >Is there a reason that function has to return unique_ptr<widget> >instead of unique_ptr<tree_widget>? The conversion to ptr-to-base >could happen at the call site (where you always have an rvalue) >instead of on the return value. Changing the return type to match the return value would avoid the issue entirely though, as it would be valid with all C++11 compilers. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [analyzer PATCH] Restore bootstrap with g++ 4.8. 2024-06-14 10:42 ` Jonathan Wakely @ 2024-06-14 10:44 ` Jonathan Wakely 2024-06-14 12:57 ` [PATCH] analyzer: Fix g++ 4.8 bootstrap without using std::move to return std::unique_ptr Jonathan Wakely 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-06-14 10:44 UTC (permalink / raw) To: Roger Sayle; +Cc: gcc-patches, 'David Malcolm' On 14/06/24 11:42 +0100, Jonathan Wakely wrote: >On 14/06/24 11:37 +0100, Jonathan Wakely wrote: >>On 14/06/24 11:26 +0100, Jonathan Wakely wrote: >>>On 14/06/24 10:36 +0100, Jonathan Wakely wrote: >>>>On 07/06/24 19:40 +0100, Roger Sayle wrote: >>>>> >>>>>This patch restores bootstrap when using g++ 4.8 as a host compiler. >>>>>Returning a std::unique_ptr requires a std::move on C++ compilers >>>>>(pre-C++17) that don't guarantee copy elision/return value optimization. >>>> >>>>It doesn't though. The C++17 guaranteed copy elision rules are not >>>>relevant here. This is about lookup for the constructor used in the >>>>return statement, and whether that lookup considers the variable to be >>>>an lvalue or an rvalue. C++11 already says this is valid: >>>> >>>>i#include <memory> >>>> >>>>std::unique_ptr<int> f() >>>>{ >>>> std::unique_ptr<int> m; >>>> return m; >>>>} >>>> >>>>See C++11 12.8 [class.copy] p31: >>>> >>>>This elision of copy/move operations, called copy elision, is >>>>permitted in the following circumstances (which may be combined to >>>>eliminate multiple copies): >>>> >>>>- in a return statement in a function with a class return type, when >>>>the expression is the name of a non-volatile automatic object (other >>>>than a function or catch-clause parameter) with the same cv- >>>>unqualified type as the function return type, the copy/move >>>>operation can be omitted by constructing the automatic object >>>>directly into the function’s return value >>>> >>>>and then p32: >>>> >>>>When the criteria for elision of a copy operation are met or would >>>>be met save for the fact that the source object is a function >>>>parameter, and the object to be copied is designated by an lvalue, >>>>overload resolution to select the constructor for the copy is first >>>>performed as if the object were designated by an rvalue. >>>> >>>>The constructor isn't required to be elided in C++11, but the compiler >>>>is required to use a move constructor instead of a copy constructor, >>>>if a move constructor is available. So you don't need to use std::move >>>>on the return value. >>>> >>>>And the code above compiles fine with gcc 4.8.5 on CentOS 7 (and even >>>>with 4.7.1). >>>> >>>>So the std::move calls you've added are redundant, and will cause >>>>-Wredundant-move warnings. >>>> >>>>What's the error you were seeing? >>> >>>I can reproduce it: >>> >>>/home/test/src/gcc/gcc/analyzer/constraint-manager.cc: In member function ‘std::unique_ptr<text_art::widget> ana::equiv_class::make_dump_widget(const text_art::dump_widget_info&, unsigned int) const’: >>>/home/test/src/gcc/gcc/analyzer/constraint-manager.cc:1179:10: error: cannot bind ‘std::unique_ptr<text_art::tree_widget>’ lvalue to ‘std::unique_ptr<text_art::tree_widget>&&’ >>> return ec_widget; >>> ^ >>> >>>Odd ... I'm looking into it ... >> >>Ah, the problem here is that the function's return type is >>std::unique_ptr<text_art::widget> but the return value is >>std::unique_ptr<tree_widget> and that requires the converting >>constructor, not the move constructor. >> >>That *did* change after C++11, and isn't supported by G++ until 5.1 > >That changed with https://cplusplus.github.io/CWG/issues/1579.html Which for the record was implemented in r5-1576-gfb682f9458c6cf (by me, which I'd forgotten!) >which is in C++14 and is a DR against C++11, but of course compilers >that implement the original C++11 rules don't support that DR. > >Arguably, g++ should not give -Wredundant-move warnings here when >using the -std=c++11 dialect, because it's only redundant if the C++11 >compiler implements that DR. > >>Is there a reason that function has to return unique_ptr<widget> >>instead of unique_ptr<tree_widget>? The conversion to ptr-to-base >>could happen at the call site (where you always have an rvalue) >>instead of on the return value. > >Changing the return type to match the return value would avoid the >issue entirely though, as it would be valid with all C++11 compilers. > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] analyzer: Fix g++ 4.8 bootstrap without using std::move to return std::unique_ptr 2024-06-14 10:44 ` Jonathan Wakely @ 2024-06-14 12:57 ` Jonathan Wakely 2024-06-14 14:27 ` David Malcolm 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Wakely @ 2024-06-14 12:57 UTC (permalink / raw) To: gcc-patches; +Cc: David Malcolm, Roger Sayle This builds successfully using both gcc-4.8.5 and gcc-13.3.1 as host compiler. OK for trunk if testing succeeds? -- >8 -- Revert the changes in r15-1111-ge22b7f741ab54f and fix bootstrap with GCC 4.8 a different way. The original problem is not related to C++17 guaranteed copy elision, it's related to Core DR 1579 [1], which was part of C++14 but only implemented in G++ as a C++11 DR with r5-1576-gfb682f9458c6cf (so GCC 4.8 doesn't implement it). The original fix causes -Wredundant-move warnings with GCC trunk. [1] https://cplusplus.github.io/CWG/issues/1579.html gcc/analyzer/ChangeLog * constraint-manager.cc (equiv_class::make_dump_widget): Change return type to match return value and do not use std::move on return value. (bounded_ranges_constraint::make_dump_widget): Likewise. (constraint_manager::make_dump_widget): Likewise. * constraint-manager.h (equiv_class::make_dump_widget): Change return type. (bounded_ranges_constraint::make_dump_widget): Likewise. (constraint_manager::make_dump_widget): Likewise. * program-state.cc (sm_state_map::make_dump_widget): Likewise. (program_state::make_dump_widget): Likewise. * program-state.h (sm_state_map::make_dump_widget): Likewise. (program_state::make_dump_widget): Likewise. * region-model.cc (region_to_value_map::make_dump_widget): Likewise. (region_model::make_dump_widget): Likewise. * region-model.h (region_to_value_map::make_dump_widget): Likewise. (region_model::make_dump_widget): Likewise. * region.cc (region::make_dump_widget): Likewise. * region.h (region::make_dump_widget): Likewise. * store.cc (binding_cluster::make_dump_widget): Likewise. (store::make_dump_widget): Likewise. * store.h (binding_cluster::make_dump_widget): Likewise. (store::make_dump_widget): Likewise. * svalue.cc (svalue::make_dump_widget): Likewise. * svalue.h (svalue::make_dump_widget): Likewise. --- gcc/analyzer/constraint-manager.cc | 12 ++++++------ gcc/analyzer/constraint-manager.h | 6 +++--- gcc/analyzer/program-state.cc | 8 ++++---- gcc/analyzer/program-state.h | 4 ++-- gcc/analyzer/region-model.cc | 8 ++++---- gcc/analyzer/region-model.h | 4 ++-- gcc/analyzer/region.cc | 4 ++-- gcc/analyzer/region.h | 2 +- gcc/analyzer/store.cc | 8 ++++---- gcc/analyzer/store.h | 4 ++-- gcc/analyzer/svalue.cc | 4 ++-- gcc/analyzer/svalue.h | 2 +- 12 files changed, 33 insertions(+), 33 deletions(-) diff --git a/gcc/analyzer/constraint-manager.cc b/gcc/analyzer/constraint-manager.cc index a9d58c9cdcf..29539060ebd 100644 --- a/gcc/analyzer/constraint-manager.cc +++ b/gcc/analyzer/constraint-manager.cc @@ -1146,7 +1146,7 @@ equiv_class::to_json () const return ec_obj; } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> equiv_class::make_dump_widget (const text_art::dump_widget_info &dwi, unsigned id) const { @@ -1176,7 +1176,7 @@ equiv_class::make_dump_widget (const text_art::dump_widget_info &dwi, ec_widget->add_child (tree_widget::make (dwi, &pp)); } - return std::move (ec_widget); + return ec_widget; } /* Generate a hash value for this equiv_class. @@ -1491,7 +1491,7 @@ bounded_ranges_constraint::to_json () const return con_obj; } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> bounded_ranges_constraint:: make_dump_widget (const text_art::dump_widget_info &dwi) const { @@ -1500,7 +1500,7 @@ make_dump_widget (const text_art::dump_widget_info &dwi) const (tree_widget::from_fmt (dwi, nullptr, "ec%i bounded ranges", m_ec_id.as_int ())); m_ranges->add_to_dump_widget (*brc_widget.get (), dwi); - return std::move (brc_widget); + return brc_widget; } bool @@ -1829,7 +1829,7 @@ constraint_manager::to_json () const return cm_obj; } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> constraint_manager::make_dump_widget (const text_art::dump_widget_info &dwi) const { using text_art::tree_widget; @@ -1853,7 +1853,7 @@ constraint_manager::make_dump_widget (const text_art::dump_widget_info &dwi) con if (cm_widget->get_num_children () == 0) return nullptr; - return std::move (cm_widget); + return cm_widget; } /* Attempt to add the constraint LHS OP RHS to this constraint_manager. diff --git a/gcc/analyzer/constraint-manager.h b/gcc/analyzer/constraint-manager.h index 31556aebc7a..81e9c7ec035 100644 --- a/gcc/analyzer/constraint-manager.h +++ b/gcc/analyzer/constraint-manager.h @@ -273,7 +273,7 @@ public: json::object *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi, unsigned id) const; @@ -408,7 +408,7 @@ public: void add_to_hash (inchash::hash *hstate) const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi) const; equiv_class_id m_ec_id; @@ -444,7 +444,7 @@ public: json::object *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi) const; const equiv_class &get_equiv_class_by_index (unsigned idx) const diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc index cb9c3888002..c42fc752350 100644 --- a/gcc/analyzer/program-state.cc +++ b/gcc/analyzer/program-state.cc @@ -309,7 +309,7 @@ sm_state_map::to_json () const /* Make a text_art::tree_widget describing this sm_state_map, using MODEL if non-null to describe svalues. */ -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> sm_state_map::make_dump_widget (const text_art::dump_widget_info &dwi, const region_model *model) const { @@ -382,7 +382,7 @@ sm_state_map::make_dump_widget (const text_art::dump_widget_info &dwi, state_widget->add_child (tree_widget::make (dwi, pp)); } - return std::move (state_widget); + return state_widget; } /* Return true if no states have been set within this map @@ -1229,7 +1229,7 @@ program_state::to_json (const extrinsic_state &ext_state) const } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> program_state::make_dump_widget (const text_art::dump_widget_info &dwi) const { using text_art::tree_widget; @@ -1247,7 +1247,7 @@ program_state::make_dump_widget (const text_art::dump_widget_info &dwi) const state_widget->add_child (smap->make_dump_widget (dwi, m_region_model)); } - return std::move (state_widget); + return state_widget; } /* Update this program_state to reflect a top-level call to FUN. diff --git a/gcc/analyzer/program-state.h b/gcc/analyzer/program-state.h index 7e751386b21..322ca8d0f11 100644 --- a/gcc/analyzer/program-state.h +++ b/gcc/analyzer/program-state.h @@ -119,7 +119,7 @@ public: json::object *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi, const region_model *model) const; @@ -233,7 +233,7 @@ public: json::object *to_json (const extrinsic_state &ext_state) const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi) const; void push_frame (const extrinsic_state &ext_state, const function &fun); diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 7969055a59c..4d6e16cf0f4 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -258,7 +258,7 @@ region_to_value_map::to_json () const return map_obj; } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> region_to_value_map:: make_dump_widget (const text_art::dump_widget_info &dwi) const { @@ -288,7 +288,7 @@ make_dump_widget (const text_art::dump_widget_info &dwi) const sval->dump_to_pp (pp, true); w->add_child (text_art::tree_widget::make (dwi, pp)); } - return std::move (w); + return w; } /* Attempt to merge THIS with OTHER, writing the result @@ -532,7 +532,7 @@ region_model::to_json () const return model_obj; } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> region_model::make_dump_widget (const text_art::dump_widget_info &dwi) const { using text_art::tree_widget; @@ -556,7 +556,7 @@ region_model::make_dump_widget (const text_art::dump_widget_info &dwi) const m_mgr->get_store_manager ())); model_widget->add_child (m_constraints->make_dump_widget (dwi)); model_widget->add_child (m_dynamic_extents.make_dump_widget (dwi)); - return std::move (model_widget); + return model_widget; } /* Assert that this object is valid. */ diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index f57d2069b3b..4683c1a4314 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -179,7 +179,7 @@ public: json::object *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi) const; bool can_merge_with_p (const region_to_value_map &other, @@ -288,7 +288,7 @@ class region_model json::object *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi) const; void validate () const; diff --git a/gcc/analyzer/region.cc b/gcc/analyzer/region.cc index 2eabda41941..d110b0eef35 100644 --- a/gcc/analyzer/region.cc +++ b/gcc/analyzer/region.cc @@ -1076,7 +1076,7 @@ region::maybe_print_for_user (pretty_printer *pp, /* Use DWI to create a text_art::widget describing this region in a tree-like form, using PREFIX as a prefix (e.g. for field names). */ -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> region::make_dump_widget (const text_art::dump_widget_info &dwi, const char *prefix) const { @@ -1101,7 +1101,7 @@ region::make_dump_widget (const text_art::dump_widget_info &dwi, if (m_parent) w->add_child (m_parent->make_dump_widget (dwi, "parent")); - return std::move (w); + return w; } void diff --git a/gcc/analyzer/region.h b/gcc/analyzer/region.h index 211dd3458c0..ffc05e034f1 100644 --- a/gcc/analyzer/region.h +++ b/gcc/analyzer/region.h @@ -182,7 +182,7 @@ public: bool maybe_print_for_user (pretty_printer *pp, const region_model &model) const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi, const char *prefix = nullptr) const; diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc index f58b84ef946..284866c7eff 100644 --- a/gcc/analyzer/store.cc +++ b/gcc/analyzer/store.cc @@ -1451,7 +1451,7 @@ binding_cluster::to_json () const return cluster_obj; } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> binding_cluster::make_dump_widget (const text_art::dump_widget_info &dwi, store_manager *mgr) const { @@ -1489,7 +1489,7 @@ binding_cluster::make_dump_widget (const text_art::dump_widget_info &dwi, m_map.add_to_tree_widget (*cluster_widget, dwi); - return std::move (cluster_widget); + return cluster_widget; } } @@ -2710,7 +2710,7 @@ store::to_json () const return store_obj; } -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> store::make_dump_widget (const text_art::dump_widget_info &dwi, store_manager *mgr) const { @@ -2769,7 +2769,7 @@ store::make_dump_widget (const text_art::dump_widget_info &dwi, store_widget->add_child (std::move (parent_reg_widget)); } - return std::move (store_widget); + return store_widget; } /* Get any svalue bound to REG, or NULL. */ diff --git a/gcc/analyzer/store.h b/gcc/analyzer/store.h index affb6e218a6..af9ea4172a2 100644 --- a/gcc/analyzer/store.h +++ b/gcc/analyzer/store.h @@ -617,7 +617,7 @@ public: json::object *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi, store_manager *mgr) const; @@ -760,7 +760,7 @@ public: json::object *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const text_art::dump_widget_info &dwi, store_manager *mgr) const; diff --git a/gcc/analyzer/svalue.cc b/gcc/analyzer/svalue.cc index cad6b7dd3cd..c82aa107b3c 100644 --- a/gcc/analyzer/svalue.cc +++ b/gcc/analyzer/svalue.cc @@ -230,7 +230,7 @@ svalue::maybe_print_for_user (pretty_printer *pp, (a) print_dump_widget_label, to populate the text of a tree_widget, and (b) add_dump_widget_children, to add children to the tree_widget. */ -std::unique_ptr<text_art::widget> +std::unique_ptr<text_art::tree_widget> svalue::make_dump_widget (const text_art::dump_widget_info &dwi, const char *prefix) const { @@ -252,7 +252,7 @@ svalue::make_dump_widget (const text_art::dump_widget_info &dwi, add_dump_widget_children (*w, dwi); - return std::move (w); + return w; } /* If this svalue is a constant_svalue, return the underlying tree constant. diff --git a/gcc/analyzer/svalue.h b/gcc/analyzer/svalue.h index e5503a54177..bc2374fe889 100644 --- a/gcc/analyzer/svalue.h +++ b/gcc/analyzer/svalue.h @@ -107,7 +107,7 @@ public: json::value *to_json () const; - std::unique_ptr<text_art::widget> + std::unique_ptr<text_art::tree_widget> make_dump_widget (const dump_widget_info &dwi, const char *prefix = nullptr) const; -- 2.45.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] analyzer: Fix g++ 4.8 bootstrap without using std::move to return std::unique_ptr 2024-06-14 12:57 ` [PATCH] analyzer: Fix g++ 4.8 bootstrap without using std::move to return std::unique_ptr Jonathan Wakely @ 2024-06-14 14:27 ` David Malcolm 0 siblings, 0 replies; 9+ messages in thread From: David Malcolm @ 2024-06-14 14:27 UTC (permalink / raw) To: Jonathan Wakely, gcc-patches; +Cc: Roger Sayle On Fri, 2024-06-14 at 13:57 +0100, Jonathan Wakely wrote: > This builds successfully using both gcc-4.8.5 and gcc-13.3.1 as host > compiler. > > OK for trunk if testing succeeds? Yes, thanks! Dave ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-06-14 14:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-07 18:40 [analyzer PATCH] Restore bootstrap with g++ 4.8 Roger Sayle 2024-06-07 20:18 ` David Malcolm 2024-06-14 9:36 ` Jonathan Wakely 2024-06-14 10:26 ` Jonathan Wakely 2024-06-14 10:37 ` Jonathan Wakely 2024-06-14 10:42 ` Jonathan Wakely 2024-06-14 10:44 ` Jonathan Wakely 2024-06-14 12:57 ` [PATCH] analyzer: Fix g++ 4.8 bootstrap without using std::move to return std::unique_ptr Jonathan Wakely 2024-06-14 14:27 ` David Malcolm
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).