[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini marked an inline comment as done. mehdi_amini added a comment. In D81422#2096643 , @arsenm wrote: > I would also expect a simple command line flag to llvm-lit to be able to > control this, rather than having to set an environment variable Wo

[PATCH] D81422: Change filecheck default to dump input on failure

arsenm added a comment. I would also expect a simple command line flag to llvm-lit to be able to control this, rather than having to set an environment variable Comment at: mlir/test/mlir-tblgen/op-format-spec.td:1 -// RUN: mlir-tblgen -gen-op-decls -asmformat-error-is-fatal=f

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added a comment. Perhaps llvm-dev is the right place to discuss the default behavior. Regardless of that default, perhaps there should be an option that limits the dump to N lines that always encloses the first error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D81422: Change filecheck default to dump input on failure

aeubanks added a comment. FWIW I didn't see this change initially but was pleasantly surprised with the extra output when debugging a newly added (relatively small) test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81422/new/ https://reviews.llv

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2093360 , @arsenm wrote: > The amount of context printed previously was good enough for development use. > If I ever can't figure it out from the diff, I want to look at the output > completely separate from the ter

[PATCH] D81422: Change filecheck default to dump input on failure

arsenm added a comment. I'm growing to dislike the new behavior even more as I use it. I don't think restricting the dump to CHECK-LABEL prefixes will even help. The amount of context printed previously was good enough for development use. If I ever can't figure it out from the diff, I want to

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added a comment. In D81422#2090425 , @mehdi_amini wrote: > I'm thinking about a possible improvement here: we could have FileCheck dump > the input for the current CHECK-LABEL section only: it seems like it could > reduce the output drastically wh

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. I'm thinking about a possible improvement here: we could have FileCheck dump the input for the current CHECK-LABEL section only: it seems like it could reduce the output drastically while still preserving how useful the information is? Repository: rG LLVM Github

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added a comment. In D81422#2090725 , @mehdi_amini wrote: > In D81422#2090618 , @jdenny wrote: > > > In D81422#2090425 , @mehdi_amini > > wrote: > > > > > I'm thinking

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2090618 , @jdenny wrote: > In D81422#2090425 , @mehdi_amini > wrote: > > > I'm thinking about a possible improvement here: we could have FileCheck > > dump the input for the

[PATCH] D81422: Change filecheck default to dump input on failure

arsenm added a comment. In D81422#2090425 , @mehdi_amini wrote: > I'm thinking about a possible improvement here: we could have FileCheck dump > the input for the current CHECK-LABEL section only: it seems like it could > reduce the output drastically wh

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2083761 , @arsenm wrote: > I think this is a worse default for development for large tests. Maybe the issue is with large tests that needs to be broken up? To me this is a general improvement during development at m

[PATCH] D81422: Change filecheck default to dump input on failure

arsenm added a comment. In D81422#2083808 , @mehdi_amini wrote: > In D81422#2083761 , @arsenm wrote: > > > I think this is a worse default for development for large tests. > > > Maybe the issue is with large tests t

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2083882 , @arsenm wrote: > In D81422#2083808 , @mehdi_amini > wrote: > > > In D81422#2083761 , @arsenm wrote: > > > > > I think this i

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added a comment. In D81422#2083882 , @arsenm wrote: > The environment variable was removed though? I would also at least expect > this to be an option I can set at cmake time and never have to think about > again Could you set `FILECHECK_OPTS=-d

[PATCH] D81422: Change filecheck default to dump input on failure

This revision was automatically updated to reflect the committed changes. Closed by commit rGd31c9e5a46ee: Change filecheck default to dump input on failure (authored by mehdi_amini). Changed prior to commit: https://reviews.llvm.org/D81422?vs=269581&id=269631#toc Repository: rG LLVM Github

[PATCH] D81422: Change filecheck default to dump input on failure

arsenm added a comment. I think this is a worse default for development for large tests. For some generated check lines, I'm seeing many thousand of line dumps on failure, which just makes it harder to see the point it actually failed at. Can we move this default into the buildbot defaults or s

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny accepted this revision. jdenny added a comment. LGTM. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81422/new/ https://reviews.llvm.org/D81422 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini updated this revision to Diff 269581. mehdi_amini marked 4 inline comments as done. mehdi_amini edited the summary of this revision. mehdi_amini added a comment. Address @jdenny's comments: - fix the example in lit.local.cfg - Test the default value for dump-input Repository: rG L

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added inline comments. Comment at: llvm/test/FileCheck/lit.local.cfg:42 # ; RUN: %ProtectFileCheckOutput FILECHECK_OPTS=-v \ # ; RUN: FileCheck -input-file %s %s 2>&1 \ # ; RUN: | FileCheck -check-prefix TRACE %s jdenny wrote: > Please add `-

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added inline comments. Comment at: llvm/test/FileCheck/dump-input-enable.txt:77 ;-- -; Check no -dump-input, which defaults to never. ;-- The previous section

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added a comment. By the way, `-vv` combined with `-dump-input=fail` provides a lot of additional information that can be helpful when debugging. Bots could pass that via `FILECHECK_OPTS`, or it could become the default when `-dump-input` is not `never`. In the latter case, I suggest a

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added inline comments. Comment at: llvm/test/FileCheck/dump-input-enable.txt:78 ; Check no -dump-input, which defaults to never. ;-- jdenny wrote: > Are the tests in this section passing for you now

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini added a comment. In D81422#2080758 , @probinson wrote: > I don't remember the exact reasoning but I believe it had something to do > with bot logs? @jdenny or @thopre might remember. It'd be interesting to hear about it: having the bot log

[PATCH] D81422: Change filecheck default to dump input on failure

thopre accepted this revision. thopre added a comment. In D81422#2080758 , @probinson wrote: > I don't remember the exact reasoning but I believe it had something to do > with bot logs? @jdenny or @thopre might remember. I guess -dump-input just follow

[PATCH] D81422: Change filecheck default to dump input on failure

eugenis accepted this revision. eugenis added a comment. This would help debugging sanitizer failures on the bots a lot. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81422/new/ https://reviews.llvm.org/D81422 ___

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added inline comments. Comment at: llvm/utils/FileCheck/FileCheck.cpp:117 - "FILECHECK_DUMP_INPUT_ON_FAILURE environment variable.\n" - "This option is deprecated in favor of -dump-input=fail.\n")); Please mention in the patch sum

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added a comment. In D81422#2080861 , @mehdi_amini wrote: > In D81422#2080758 , @probinson wrote: > > > I don't remember the exact reasoning but I believe it had something to do > > with bot logs? @jdenny or

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini updated this revision to Diff 269385. mehdi_amini marked 3 inline comments as done. mehdi_amini added a comment. Herald added a subscriber: delcypher. Fix more tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81422/new/ https://revie

[PATCH] D81422: Change filecheck default to dump input on failure

jpienaar accepted this revision. jpienaar added a comment. I think this is a good default given it provides very useful info in failure case. For cases where folks expect it to fail + don't want it logged, then setting never (if that is a concern) seems better behavior IMHO. Repository: rG L

[PATCH] D81422: Change filecheck default to dump input on failure

mehdi_amini updated this revision to Diff 269342. mehdi_amini added a comment. Herald added subscribers: Sanitizers, cfe-commits, msifontes, jurahul, Kayjukh, frgossen, grosul1, Joonsoo, stephenneuendorffer, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, csigg, nicolasvasilache, antiagainst

[PATCH] D81422: Change filecheck default to dump input on failure

jdenny added a comment. Also, please grep for `FILECHECK_DUMP_INPUT_ON_FAILURE`. There are a few more occurrences to remove from the repo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81422/new/ https://reviews.llvm.org/D81422 ___

[PATCH] D81422: Change filecheck default to dump input on failure

aartbik accepted this revision. aartbik added a comment. This revision is now accepted and ready to land. Yes, assuming nobody remembers an adverse side-effect, I think this change will help debugging future failures. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re