On 02/26/2018 11:19 PM, Martin Sebor wrote:
While reviewing other related bugs I noticed 83502. This patch
doesn't fix the first test case in the bug (attribute noinline
vs always_inline). Somehow those are still copied from
the primary to the specialization and can cause conflicts.
Hmm, that's odd. Why is that?
Because duplicate_decl calls diagnose_mismatched_attributes()
on the NEWDECL and OLDDECL. (Attribute optimize would do the
same thing.) I was trying to keep the fix small but it makes
sense to take care of this as well so I have in this revision.
It does fix the second test case but with the noreturn change
it would issue a bogus -Wmissing-attributes warning for the
explicit specialization below. Adding the warn_unused_result
attribute to it would then make GCC complain about a conflict
between the added attribute and noreturn, while removing it
would lead to worse code.
template <class T>
int __attribute__ ((warn_unused_result)) f (T) { return 0; }
template <>
int __attribute__ ((noreturn)) f<int> (int) { throw 0; }
void fi () { f (0); }
I continue to disagree with this use of attribute noreturn.
+ /* Merge the function-has-side-effects bit. */
+ if (TREE_THIS_VOLATILE (newdecl))
+ TREE_THIS_VOLATILE (olddecl) = 1;
+
+ if (merge_attr)
TREE_THIS_VOLATILE means attribute noreturn, not whether the function
has side-effects; it should be handled in the blocks controlled by
merge_attr.
Whoops. That was a silly goof. I must have misread the comment
above the macro definition. I also didn't have a test for it (or
some of the other changes I've made) so I didn't see the problem.
Attached is an enhanced version of the patch that handles (and
tests) more of the commonly used attributes. I'm not sure why
in the merge_attr block I have to merge TREE_THIS_VOLATILE and
TREE_NOTHROW back and forth but not also READONLY, PURE, or
MALLOC, but without it tests fail.
Because the memcpy from newdecl to olddecl at the end of duplicate_decls
explicitly excludes the tree_common section. I don't know why that is,
it certainly complicates the logic. That choice seems to predate the
C++ front end.
PS Would it be possible to add a new macro with "noreturn" in
the name to make it more intuitive? (And ditto perhaps also
for TREE_READONLY for "const" functions, though for whatever
reason that seems easier to decipher. I know you're all used
to it but it's far from intuitive.)
Sounds good.
PPS Duplicate_decls is over 1,400 lines long. If there is more
work to do here in stage 1 (I suspect there might be), would you
mind if I broke it up into two or more, say one for functions,
another for types, or whatever grouping makes most sense to make
it easier to follow?
Sure, there's plenty of scope for cleaning up duplicate_decls. :)
+ else
{
if (DECL_DECLARED_INLINE_P (newdecl))
DECL_DISREGARD_INLINE_LIMITS (newdecl) = true;
This looks like it will mean setting DECL_DISREGARD_INLINE_LIMITS on all
inline template specializations. I think you want to drop these two lines.
OK with that change.
Jason
