llvm dev - Nov 2013 - [LLVMdev] [PATCH] Prevent clang from throwing the diagnostics twice.

Hi All,

Clang pop up with the  "error: invalid integral value" diagnostics 
twice
,when you enable the optimization  through  -O with a  non-integer value
 i.e -O<non integer>  as show below

$ clang -Of -S test.c
error: invalid integral value 'f' in '-Of'
error: invalid integral value 'f' in '-Of'


Attached patch fix the issue as

$ clang -Of -S test.c
error: invalid integral value 'f' in '-Of'


Clang version
clang version 3.4
(http://llvm.org/git/clang.git26297f57634994b4ae47a0774c372d6944265bb2)
(
http://llvm.org/git/llvm.git 868e74bdce7a2c49d60e2ef56a077b0aa7f2ba08)

Target: i386-pc-linux-gnu
Thread model: posix

Builded successfully on trunk and is it ok to commit ??


Thanks
~Umesh
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131115/728e6fce/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_changes.patch
Type: application/octet-stream
Size: 1003 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131115/728e6fce/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_changes.patch
Type: application/octet-stream
Size: 2066 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131115/728e6fce/attachment-0001.obj>

Test cases?

Also, this seems a bit heavy-handed for a solution. Why is
getLastArgIntValue being called twice in the first place?

~Aaron

On Fri, Nov 15, 2013 at 6:35 AM, Umesh Kalappa <umesh.kalappa0 at
gmail.com> wrote:
> Hi All,
>
> Clang pop up with the  "error: invalid integral value"
diagnostics  twice
> ,when you enable the optimization  through  -O with a  non-integer value
> i.e -O<non integer>  as show below
>
> $ clang -Of -S test.c
> error: invalid integral value 'f' in '-Of'
> error: invalid integral value 'f' in '-Of'
>
>
> Attached patch fix the issue as
>
> $ clang -Of -S test.c
> error: invalid integral value 'f' in '-Of'
>
>
> Clang version
> clang version 3.4 (http://llvm.org/git/clang.git
> 26297f57634994b4ae47a0774c372d6944265bb2) (http://llvm.org/git/llvm.git
> 868e74bdce7a2c49d60e2ef56a077b0aa7f2ba08)
>
> Target: i386-pc-linux-gnu
> Thread model: posix
>
> Builded successfully on trunk and is it ok to commit ??
>
>
> Thanks
> ~Umesh
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

Hi Umesh,
Should the call to StringRef::getAsInteger() be predicated on the
A->getIssuedDiagnostics() call?

I assume the common case would be that we don't emit a diagnostic and thus
the below would be a faster implementation:

if (StringRef(A->getValue()).getAsInteger(10, Res)) {
  if (Diags) {
    if (A->getIssuedDiagnostics() ==  false){
      A->setIssuedDiagnostics(true);
      Diags->Report(diag::err_drv_invalid_int_value) <<
A->getAsString(Args)
                                                     << A->getValue();
    }
  }
}

 Chad
> Hi All,
>
> Clang pop up with the  "error: invalid integral value"
diagnostics  twice
> ,when you enable the optimization  through  -O with a  non-integer value
>  i.e -O<non integer>  as show below
>
> $ clang -Of -S test.c
> error: invalid integral value 'f' in '-Of'
> error: invalid integral value 'f' in '-Of'
>
>
> Attached patch fix the issue as
>
> $ clang -Of -S test.c
> error: invalid integral value 'f' in '-Of'
>
>
> Clang version
> clang version 3.4
> (http://llvm.org/git/clang.git26297f57634994b4ae47a0774c372d6944265bb2)
> (
> http://llvm.org/git/llvm.git 868e74bdce7a2c49d60e2ef56a077b0aa7f2ba08)
>
> Target: i386-pc-linux-gnu
> Thread model: posix
>
> Builded successfully on trunk and is it ok to commit ??
>
>
> Thanks
> ~Umesh
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

> Test cases?
Yes, please.
> Also, this seems a bit heavy-handed for a solution. Why is
> getLastArgIntValue being called twice in the first place?
>
I tend to agree with Aaron here.  However, I can't think of a better
solution.  Perhaps the issue is that getOptimizationLevel() is being
called multiple times?  The call happens in two different static functions
and I don't see a way to easily refactor the code.

 Chad
> ~Aaron
>
> On Fri, Nov 15, 2013 at 6:35 AM, Umesh Kalappa <umesh.kalappa0 at
gmail.com>
> wrote:
>> Hi All,
>>
>> Clang pop up with the  "error: invalid integral value"
diagnostics
>> twice
>> ,when you enable the optimization  through  -O with a  non-integer
value
>> i.e -O<non integer>  as show below
>>
>> $ clang -Of -S test.c
>> error: invalid integral value 'f' in '-Of'
>> error: invalid integral value 'f' in '-Of'
>>
>>
>> Attached patch fix the issue as
>>
>> $ clang -Of -S test.c
>> error: invalid integral value 'f' in '-Of'
>>
>>
>> Clang version
>> clang version 3.4 (http://llvm.org/git/clang.git
>> 26297f57634994b4ae47a0774c372d6944265bb2) (http://llvm.org/git/llvm.git
>> 868e74bdce7a2c49d60e2ef56a077b0aa7f2ba08)
>>
>> Target: i386-pc-linux-gnu
>> Thread model: posix
>>
>> Builded successfully on trunk and is it ok to commit ??
>>
>>
>> Thanks
>> ~Umesh
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

On Fri, Nov 15, 2013 at 11:19 AM, Umesh Kalappa
<umesh.kalappa0 at gmail.com> wrote:
> Hi Aaron,
>
>>>Why is getLastArgIntValue being called twice in the first place?
>
> It has been called from the getOptimizationLevel() ,which in turn called by
> methods like ParseCodeGenArgs() and ParseLangArgs(),
>
> where method getOptimizationLevel() register the "invalid integral
value"
> diagnostics in the diagnostics engine twice ,since it was called twice.
>
> So i added the flag to check if the diagnostics is already registered for a
> specific argument then don't add again and for first time set the flag.
>
> Any suggestion are welcome .
As Chad pointed, it's likely that this requires a non-trivial
refactoring. I don't have particularly bright ideas on solving it, but
I'm uncomfortable adding the flag at the arg level because it kind of
encourages lazy design when handling args. Perhaps looking into ways
the code could be refactored so that getOptimizationLevel() is only
called once (and perhaps the results get cached somewhere) would be a
good approach.

~Aaron

> As Chad pointed, it's likely that this requires a non-trivial
> refactoring. I don't have particularly bright ideas on solving it, but
> I'm uncomfortable adding the flag at the arg level because it kind of
> encourages lazy design when handling args. Perhaps looking into ways
> the code could be refactored so that getOptimizationLevel() is only
> called once (and perhaps the results get cached somewhere) would be a
> good approach.
I completely agree with you, Aaron.

 Chad