thr3ads.net - llvm dev - [LLVMdev] anchoring explicit template instantiations [Dec 2011]

If this information is useful, please help other people find it:
Share via:

David Blaikie

2011-Dec-01 08:08 UTC

[LLVMdev] anchoring explicit template instantiations

On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <clattner at apple.com>
wrote:> On Nov 29, 2011, at 12:26 AM, David Blaikie wrote:
>> For a bit of an experiment I've been trying to compile LLVM &
Clang
>> with -Weverything (disabling any errors that seem like more noise/less
>> interesting). One warning I've recently hit a few instances of is
>> -Wweak-vtable which is, in fact, an explicitly documented LLVM coding
>> standard ( http://llvm.org/docs/CodingStandards.html#ll_virtual_anch
>> ). Some instances of this have been easy to fix (see attached patch -
>> if it looks good to anyone I'll check that in) but a particular set
of
>> them have been a little more problematic.
>
> Nice, please commit your patch.  I don't know about explicit
instantiations though, maybe a C++ guru on the clang list knows.
Thanks Chris, committed as r145578. I don't suppose you'll mind some
similar commits as I encounter them?

(there's also some legitimate unreachable code warnings I'd be happy
to fix as I find them, things like:

--- a/lib/Support/CommandLine.cpp
+++ b/lib/Support/CommandLine.cpp
@@ -294,10 +294,7 @@ static inline bool ProvideOption(Option *Handler,
StringRef ArgName,
     break;

   default:
-    errs() << ProgramName
-         << ": Bad ValueMask flag! CommandLine usage error:"
-         << Handler->getValueExpectedFlag() << "\n";
-    llvm_unreachable(0);
+    llvm_unreachable("Bad ValueMask flag!");
   }


and

--- a/lib/Support/APInt.cpp
+++ b/lib/Support/APInt.cpp
@@ -1440,16 +1440,14 @@ APInt APInt::sqrt() const {
   APInt nextSquare((x_old + 1) * (x_old +1));
   if (this->ult(square))
     return x_old;
-  else if (this->ule(nextSquare)) {
+  if (this->ule(nextSquare)) {
     APInt midpoint((nextSquare - square).udiv(two));
     APInt offset(*this - square);
     if (offset.ult(midpoint))
       return x_old;
-    else
-      return x_old + 1;
-  } else
-    llvm_unreachable("Error in APInt::sqrt computation");
-  return x_old + 1;
+    return x_old + 1;
+  }
+  llvm_unreachable("Error in APInt::sqrt computation");
 }

(a return after an llvm_unreachable - muddied up with a bit of
syntactic tidyup))

& I'll take the detailed discussion about -Wweak-vtables for template
instantiations to cfe-dev & see what they have to say.

- David

Chris Lattner

2011-Dec-01 17:11 UTC

head link

[LLVMdev] anchoring explicit template instantiations

On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:
> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <clattner at
apple.com> wrote:
>> On Nov 29, 2011, at 12:26 AM, David Blaikie wrote:
>>> For a bit of an experiment I've been trying to compile LLVM
& Clang
>>> with -Weverything (disabling any errors that seem like more
noise/less
>>> interesting). One warning I've recently hit a few instances of
is
>>> -Wweak-vtable which is, in fact, an explicitly documented LLVM
coding
>>> standard (
http://llvm.org/docs/CodingStandards.html#ll_virtual_anch
>>> ). Some instances of this have been easy to fix (see attached patch
-
>>> if it looks good to anyone I'll check that in) but a particular
set of
>>> them have been a little more problematic.
>> 
>> Nice, please commit your patch.  I don't know about explicit
instantiations though, maybe a C++ guru on the clang list knows.
> 
> Thanks Chris, committed as r145578. I don't suppose you'll mind
some
> similar commits as I encounter them?
Yep, please feel free.
> (there's also some legitimate unreachable code warnings I'd be
happy
> to fix as I find them, things like:
> 
> --- a/lib/Support/CommandLine.cpp
> +++ b/lib/Support/CommandLine.cpp
> @@ -294,10 +294,7 @@ static inline bool ProvideOption(Option *Handler,
> StringRef ArgName,
>     break;
> 
>   default:
> -    errs() << ProgramName
> -         << ": Bad ValueMask flag! CommandLine usage
error:"
> -         << Handler->getValueExpectedFlag() <<
"\n";
> -    llvm_unreachable(0);
> +    llvm_unreachable("Bad ValueMask flag!");
This patch would lose the expected flag value, which is unfortunate.
> +++ b/lib/Support/APInt.cpp
> @@ -1440,16 +1440,14 @@ APInt APInt::sqrt() const {
>   APInt nextSquare((x_old + 1) * (x_old +1));
>   if (this->ult(square))
>     return x_old;
> +  if (this->ule(nextSquare)) {
>     APInt midpoint((nextSquare - square).udiv(two));
>     APInt offset(*this - square);
>     if (offset.ult(midpoint))
>       return x_old;
> +  }
> +  llvm_unreachable("Error in APInt::sqrt computation");
> }
> 
> (a return after an llvm_unreachable - muddied up with a bit of
> syntactic tidyup))
This is an abuse of llvm_unreachable.  It should just replace "if ule"
check with:

assert(this->ule(nextSquare) && "Error in APInt::sqrt
computation");
> & I'll take the detailed discussion about -Wweak-vtables for
template
> instantiations to cfe-dev & see what they have to say.
Thanks David!

-Chris

David Blaikie

2011-Dec-01 21:13 UTC

head link

[LLVMdev] anchoring explicit template instantiations

On Thu, Dec 1, 2011 at 9:11 AM, Chris Lattner <clattner at apple.com>
wrote:>
> On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:
>
>> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <clattner at
apple.com> wrote:
>>> On Nov 29, 2011, at 12:26 AM, David Blaikie wrote:
>>>> For a bit of an experiment I've been trying to compile LLVM
& Clang
>>>> with -Weverything (disabling any errors that seem like more
noise/less
>>>> interesting). One warning I've recently hit a few instances
of is
>>>> -Wweak-vtable which is, in fact, an explicitly documented LLVM
coding
>>>> standard (
http://llvm.org/docs/CodingStandards.html#ll_virtual_anch
>>>> ). Some instances of this have been easy to fix (see attached
patch -
>>>> if it looks good to anyone I'll check that in) but a
particular set of
>>>> them have been a little more problematic.
>>>
>>> Nice, please commit your patch.  I don't know about explicit
instantiations though, maybe a C++ guru on the clang list knows.
>>
>> Thanks Chris, committed as r145578. I don't suppose you'll mind
some
>> similar commits as I encounter them?
>
> Yep, please feel free.
Thanks - I'll see what comes up.
>> (there's also some legitimate unreachable code warnings I'd be
happy
>> to fix as I find them, things like:
>>
>> --- a/lib/Support/CommandLine.cpp
>> +++ b/lib/Support/CommandLine.cpp
>> @@ -294,10 +294,7 @@ static inline bool ProvideOption(Option *Handler,
>> StringRef ArgName,
>>     break;
>>
>>   default:
>> -    errs() << ProgramName
>> -         << ": Bad ValueMask flag! CommandLine usage
error:"
>> -         << Handler->getValueExpectedFlag() <<
"\n";
>> -    llvm_unreachable(0);
>> +    llvm_unreachable("Bad ValueMask flag!");
>
> This patch would lose the expected flag value, which is unfortunate.
It is unfortunate - though I'm not sure whether you're suggesting that
the change should be made anyway, or not.

I'd say the whole unreachable could be dropped & an assertion placed
inside getValueExpectedFlag which is where the actual unsafe bit->enum
conversion is occuring.
(that would still leave the need for a default in this switch because
one of the values of the enum isn't covered - ValueMask, but really
that probably shouldn't be one of the enumeration values anyway,
should it? (looks like it should just be a hidden constant somewhere
in the implementation details of llvm::Option))
>> +++ b/lib/Support/APInt.cpp
>> @@ -1440,16 +1440,14 @@ APInt APInt::sqrt() const {
>>   APInt nextSquare((x_old + 1) * (x_old +1));
>>   if (this->ult(square))
>>     return x_old;
>> +  if (this->ule(nextSquare)) {
>>     APInt midpoint((nextSquare - square).udiv(two));
>>     APInt offset(*this - square);
>>     if (offset.ult(midpoint))
>>       return x_old;
>> +  }
>> +  llvm_unreachable("Error in APInt::sqrt computation");
>> }
>>
>> (a return after an llvm_unreachable - muddied up with a bit of
>> syntactic tidyup))
>
> This is an abuse of llvm_unreachable.  It should just replace "if
ule" check with:
Ah, so it is. I hadn't noticed (was just looking at the simple
transformation without actually parsing the semantics any further).

Committed with assert instead of abusive unreachable in r145627

Thanks,
- David

David Blaikie

2011-Dec-11 01:20 UTC

head link

[LLVMdev] anchoring explicit template instantiations

On Thu, Dec 1, 2011 at 9:11 AM, Chris Lattner <clattner at apple.com>
wrote:>
> On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:
>
>> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <clattner at
apple.com> wrote:
>>> On Nov 29, 2011, at 12:26 AM, David Blaikie wrote:
>>>> For a bit of an experiment I've been trying to compile LLVM
& Clang
>>>> with -Weverything (disabling any errors that seem like more
noise/less
>>>> interesting). One warning I've recently hit a few instances
of is
>>>> -Wweak-vtable which is, in fact, an explicitly documented LLVM
coding
>>>> standard (
http://llvm.org/docs/CodingStandards.html#ll_virtual_anch
>>>> ). Some instances of this have been easy to fix (see attached
patch -
>>>> if it looks good to anyone I'll check that in) but a
particular set of
>>>> them have been a little more problematic.
>>>
>>> Nice, please commit your patch.  I don't know about explicit
instantiations though, maybe a C++ guru on the clang list knows.
>>
>> Thanks Chris, committed as r145578. I don't suppose you'll mind
some
>> similar commits as I encounter them?
>
> Yep, please feel free.
While you said this - given that I've now gone & fixed /every/
violation of -Wweak-vtables across LLVM & Clang (apart from some llvm
target tblgen problems - not sure how practical they are to fix. And
gtest also fires this warning) I thought I should probably get at
least a '*nod*' before I check this in.

Actually it'd be kind of interesting to see how much/if any difference
in compile time this actually makes. I do wonder.

(also, I implemented this by adding private anchors, like my original
version - this does actually have a difference at runtime, of course -
since now each of these types has another entry in their vtable.
Alternatively I could've used their destructors (but then they
wouldn't be inline anymore - but probably most of them are called
virtually anyway so their inline-ness doesn't matter). Also I had to
add about 10 source files in total as implementation files for
header-only cases that needed anchors)

Also - do we have anywhere we could put standard flags that LLVM
should successfully compile with? At least if clang is being used,
perhaps? So we could add -Wweak-vtables -Werror for this case & add
more as we deem it appropriate.

- David
>> (there's also some legitimate unreachable code warnings I'd be
happy
>> to fix as I find them, things like:
>>
>> --- a/lib/Support/CommandLine.cpp
>> +++ b/lib/Support/CommandLine.cpp
>> @@ -294,10 +294,7 @@ static inline bool ProvideOption(Option *Handler,
>> StringRef ArgName,
>>     break;
>>
>>   default:
>> -    errs() << ProgramName
>> -         << ": Bad ValueMask flag! CommandLine usage
error:"
>> -         << Handler->getValueExpectedFlag() <<
"\n";
>> -    llvm_unreachable(0);
>> +    llvm_unreachable("Bad ValueMask flag!");
>
> This patch would lose the expected flag value, which is unfortunate.
>
>> +++ b/lib/Support/APInt.cpp
>> @@ -1440,16 +1440,14 @@ APInt APInt::sqrt() const {
>>   APInt nextSquare((x_old + 1) * (x_old +1));
>>   if (this->ult(square))
>>     return x_old;
>> +  if (this->ule(nextSquare)) {
>>     APInt midpoint((nextSquare - square).udiv(two));
>>     APInt offset(*this - square);
>>     if (offset.ult(midpoint))
>>       return x_old;
>> +  }
>> +  llvm_unreachable("Error in APInt::sqrt computation");
>> }
>>
>> (a return after an llvm_unreachable - muddied up with a bit of
>> syntactic tidyup))
>
> This is an abuse of llvm_unreachable.  It should just replace "if
ule" check with:
>
> assert(this->ule(nextSquare) && "Error in APInt::sqrt
computation");
>
>> & I'll take the detailed discussion about -Wweak-vtables for
template
>> instantiations to cfe-dev & see what they have to say.
>
> Thanks David!
>
> -Chris-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-weak-vtables.diff
Type: text/x-diff
Size: 77820 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111210/b7de31d0/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-weak-vtables.diff
Type: text/x-diff
Size: 106686 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111210/b7de31d0/attachment-0001.diff>

Reasonably Related Threads

Search for more reasonably related threads

llvm dev - Dec 2011 - [LLVMdev] anchoring explicit template instantiations

[LLVMdev] anchoring explicit template instantiations

[LLVMdev] anchoring explicit template instantiations

[LLVMdev] anchoring explicit template instantiations

[LLVMdev] anchoring explicit template instantiations

Reasonably Related Threads