[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. Thank you for your clarification. I'll change it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105169/new/ https://reviews.llvm.org/D105169 ___ cfe-commits mailing list

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

MaskRay added a comment. In D105169#3321237 , @hyeongyukim wrote: > In D105169#3319773 , @dblaikie > wrote: > >> In D105169#3315009 , @MaskRay >> wrote: >> >>> It may

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. In D105169#3319773 , @dblaikie wrote: > In D105169#3315009 , @MaskRay wrote: > >> It may not be worth changing now, but I want to mention: it's more >> conventional to have a

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

dblaikie added a comment. In D105169#3315009 , @MaskRay wrote: > It may not be worth changing now, but I want to mention: it's more > conventional to have a `BoolOption` which adds `-[no-]noundef-analysis`. > Since both positive and negative forms

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

MaskRay added a comment. It may not be worth changing now, but I want to mention: it's more conventional to have a `BoolOption` which adds `-[no-]noundef-analysis`. Since both positive and negative forms exist. When we make the default switch, existing users don't need to change the option.

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } nlopes wrote: > sammccall wrote: > > fhahn wrote: > > > nlopes wrote: > > > >

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nlopes added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } sammccall wrote: > fhahn wrote: > > nlopes wrote: > > > fhahn wrote: > > > >

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nlopes added a comment. In D105169#3257849 , @sdesmalen wrote: > In D105169#3254757 , @nlopes wrote: > >> In D105169#3254692 , >> @DavidSpickett wrote: >> >>> Hi, this

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

sdesmalen added a comment. In D105169#3254757 , @nlopes wrote: > In D105169#3254692 , @DavidSpickett > wrote: > >> Hi, this patch has caused a gcc test suite failure on our SVE bots: >>

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

sammccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } fhahn wrote: > nlopes wrote: > > fhahn wrote: > > > nlopes wrote: > > > > ab

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } nlopes wrote: > fhahn wrote: > > nlopes wrote: > > > ab wrote: > > > > Hmm, if

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nlopes added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } fhahn wrote: > nlopes wrote: > > ab wrote: > > > Hmm, if I'm reading this right,

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

fhahn added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } nlopes wrote: > ab wrote: > > Hmm, if I'm reading this right, this overwrites the

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nlopes added a comment. In D105169#3254692 , @DavidSpickett wrote: > Hi, this patch has caused a gcc test suite failure on our SVE bots: > https://lab.llvm.org/buildbot/#/builders/184/builds/1941 > (test source is >

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

DavidSpickett added a comment. Hi, this patch has caused a gcc test suite failure on our SVE bots: https://lab.llvm.org/buildbot/#/builders/184/builds/1941 (test source is https://github.com/llvm/llvm-test-suite/blob/main/SingleSource/Regression/C/gcc-c-torture/execute/pr79286.c) I've only

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nlopes added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } ab wrote: > Hmm, if I'm reading this right, this overwrites the `nonnull

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

ab added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2535 ArgAttrs[FirstIRArg + i] = llvm::AttributeSet::get(getLLVMContext(), Attrs); } Hmm, if I'm reading this right, this overwrites the `nonnull dereferenceable align`

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

vitalybuka added a comment. In D105169#3248124 , @sammccall wrote: > Ah, this seems to be coming from the buildbot configuration. > >

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

sammccall added a subscriber: vitalybuka. sammccall added a comment. Ah, this seems to be coming from the buildbot configuration. https://github.com/llvm/llvm-zorg/blob/38ab06456f7302b4eab53e5b6f1e2eaf4f127132/zorg/buildbot/builders/sanitizers/buildbot_functions.sh#L140-L149 @vitalybuka looks

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

sammccall added a comment. This appears to have triggered some buildbot failures: https://lab.llvm.org/buildbot/#/builders/5/builds/17461/steps/10/logs/stdio e.g. in path-mappings.test: "message": "Unknown argument: '-enable-noundef-analysis'", This flag isn't being added by clangd code,

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nathanchance added a comment. In D105169#3246378 , @nlopes wrote: > In D105169#3244624 , @nathanchance > wrote: > >> I can reduce all of these down for you and/or I can start an email thread >> with the

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nlopes added a comment. In D105169#3244624 , @nathanchance wrote: > I can reduce all of these down for you and/or I can start an email thread > with the `objtool` maintainers to see if there is a way to fix or avoid these > warnings on the `objtool`

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nathanchance added a subscriber: nickdesaulniers. nathanchance added a comment. In D105169#3242949 , @hyeongyukim wrote: > @nathanchance Hi, I analyzed all four warnings. Thanks a lot for doing this! > Warning #1: can be handled by x86-backend. filled

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. @nathanchance Hi, I analyzed all four warnings. Warning #1: can be handled by x86-backend. filled issue #53118 Warning #2: bug in the kernel, fixed in the next version. Warning #3: same reason with #2 Warning #4:

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. @nathanchance You were right. I succeeded in reproducing the warning with `-fsanitize-coverage=trace-pc` However, this problem seems to be fixed in the latest version. (I used `57a551a`) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nathanchance added a comment. In D105169#3236484 , @hyeongyukim wrote: > @nathanchance > > I tried to reproduce the last warning (intelfbhw_validate_mode), but I failed > to produce it. > I think my reproducer is correct, but it does not make any

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. @nathanchance I tried to reproduce the last warning (intelfbhw_validate_mode), but I failed to produce it. I think my reproducer is correct, but it does not make any warning. Can you tell me which part was wrong? clang -O2 -flto=thin

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. Great. I'll check it out. > I have a reproducer for the first two, as that is all I had time for; if you > would like them for the other two, I can get those for you tomorrow. @nathanchance I think the other two can be reproduced without difficulty. If the

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nathanchance added a comment. @hyeongyukim I hand reduced a couple of the translation units that I see issues in so apologies if they are a little verbose. The full set of warnings: drivers/net/ethernet/renesas/ravb.lto.o: warning: objtool: .text.ravb_set_gti: unexpected end of section

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. In D105169#3211929 , @nathanchance wrote: > @hyeongyukim I am currently offline for the evening but it seems like my > reduction might have been too aggressive. It looks like this code comes from > `ravb_set_gti()` >

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nathanchance added a comment. @hyeongyukim I am currently offline for the evening but it seems like my reduction might have been too aggressive. It looks like this code comes from `ravb_set_gti()`

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. In D105169#3116810 , @nathanchance wrote: > Prior to the latest revert (fd9b099906c61e46574d1ea2d99b973321fe1d21 > ), the > Linux kernel's binary

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

erichkeane added a comment. In D105169#3130355 , @hyeongyukim wrote: > In D105169#3115814 , @erichkeane > wrote: > >> Either this or D108453 (which were >> committed

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. In D105169#3115814 , @erichkeane wrote: > Either this or D108453 (which were > committed together!) caused this assert according to my git-bisect: > https://godbolt.org/z/4rqYKfW7K > >

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nathanchance added a comment. Prior to the latest revert (fd9b099906c61e46574d1ea2d99b973321fe1d21 ), the Linux kernel's binary verifier (`objtool`) points out an issue when building with ThinLTO and

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

rjmccall added a comment. Hmm. I wonder if that's related to the problem uncovered by the verifier in https://reviews.llvm.org/D113352. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105169/new/ https://reviews.llvm.org/D105169

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. In D105169#3115814 , @erichkeane wrote: > Either this or D108453 (which were > committed together!) caused this assert according to my git-bisect: > https://godbolt.org/z/4rqYKfW7K > >

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

erichkeane added a comment. Either this or D108453 (which were committed together!) caused this assert according to my git-bisect: https://godbolt.org/z/4rqYKfW7K NOTE that this fails in a lit-test for me, clang CodeGen/ifunc.c (though my downstream seems to

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

eugenis added a comment. In D105169#3111935 , @hyeongyukim wrote: > - long long res; > + register long long res __asm__("x0"); > > Is it okay to commit this change by myself? Yes, go ahead! Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index ea3e5bdbc754..826c6d36e1b1 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. In D105169#3104262 , @eugenis wrote: > You are absolutely right. X86 variant uses an "=a" constraint (rax register), > others pin the output variable to a specific register with __asm__ > declaration. It appears we've missed it

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

eugenis added a comment. You are absolutely right. X86 variant uses an "=a" constraint (rax register), others pin the output variable to a specific register with __asm__ declaration. It appears we've missed it in Aarch64. Could you check if __asm__("x0") in the declaration of res helps?

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. I am not familiar with inline assembly, but it seems the output variable (`%0`) is not updated because it does not appear in the code. 1382 __asm__ __volatile__( 1383"mov x0,x2\n" /* flags */ 1384"mov x2,x4\n" /*

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. I checked the reason for failure in address sanitizer tests on the 2-stage aarch64 buildbots. The buildbot failure was occured because the `internal_clone` function of the `compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp` file is being compiled incorrectly.

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. I will revert this patch, since its fix needs some time for me to investigate. I have access to an AArch server, so I can give it a try by myself. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105169/new/

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

sylvestre.ledru added a comment. This change caused libclc to fail to build on old ubuntu (using llvm-spir 10.0.0-1) See: https://bugs.llvm.org/show_bug.cgi?id=52200 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105169/new/

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

ostannard added a comment. This change is causing a lot of failures in the address sanitiser tests on the 2-stage AArch64 buildbots. For example: https://lab.llvm.org/buildbot/#/builders/179/builds/1326 I can reproduce the failures on another AArch64 machine, they only happen with a 2-stage

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

mstorsjo added a comment. In D105169#3069555 , @aqjune wrote: > I see, the crash is happening at the line because SimplifyCFG removes the > `sti->index_entries` null pointer check (which seems valid to me). > If `stl->index_entries` was null, it would

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. In D105169#3069417 , @mstorsjo wrote: > Yes, I believe so. If the branch is not taken, `pos_min` and `pos_max` are > undefined when entering `ff_gen_search`. (I would assume that their value > isn't ever used within

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

mstorsjo added a comment. In D105169#3069220 , @aqjune wrote: > It seems the original code has a use of an uninitialized variable. > Line 4420 at seek-preproc.c (function `ff_seek_frame_binary`): > >int64_t pos_min=pos_min, pos_max=pos_max, pos,

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. It seems the original code has a use of an uninitialized variable. Line 4420 at seek-preproc.c (function `ff_seek_frame_binary`): int64_t pos_min=pos_min, pos_max=pos_max, pos, pos_limit; // pos_min and pos_max are self-assigned. ... if (sti->index_entries) {

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. I can see that `@ff_seek_frame_binary` is the only affected function. It introduces `llvm.assume` as well as `!nonnull` at a few places and folds null pointer checks. Still investigating.. F19677416: before.ll F19677415: after.ll

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

mstorsjo added a comment. I've bisected a crash in generated code down to this commit. The code that crashes is clean when run in ubsan. The misbehaviour happens across 4 tested architectures (i686, x86_64, armv7, aarch64). The misbehaviour occurs in this preprocessed source,

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG80dba72a669b: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and… (authored by aqjune, committed by hyeongyukim). Changed

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. There has been no concern in a week, so I'd like to land this patch and D108453 in this weekend. I'll carefully watch the buildbots and address failures if any. @eugenis could you accept this patch and D108453

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. Thank you! In the meantime, I will rebase this patch and resolve conflicts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105169/new/ https://reviews.llvm.org/D105169 ___

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. Thank you! I'll send a mail to cfe-dev + llvm-dev to notify the change (and gather any possible concern if exists). If there is no objection in a week, let's land these patches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

eugenis added a comment. @hyeongyukim, thank you for the summary. This looks like a great change, and a net positive to me. The test churn in the other patch is unfortunate, but IMHO unavoidable. If no one has any further objections, LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

hyeongyukim added a comment. I did some experiments to confirm the benefits of adding a noundef attribute to the function parameter. (test result link ) One of the most significant advantages of

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

nlopes added a comment. (repost my message from llvm-dev) I can add one thought regarding why this direction makes sense and why it doesn't constrain optimizations. Traditionally we don't want to mark too many things as UB as it restricts code movement and thus limits optimizations. That's

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

aqjune added a comment. Hello all, I'm a messenger of @hyeongyukim's two patches that allow clang to turn on noundef analysis by default. The noundef analysis flag was added by D81678 in the past. Its goal is to mark arguments and return values in C/C++ as