[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #1 from roger.li...@cedalo.com --- Created attachment 113910 --> https://bugs.kde.org/attachment.cgi?id=113910&action=edit Patch using sha3 for hashing instead of adler32 checksums. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 Philippe Waroquiers changed: What|Removed |Added CC||philippe.waroquiers@skynet. ||be --- Comment #2 from Philippe Waroquiers --- I took a (very) quick look at the patch. IIUC, the tool stores a sha3 of the stack traces for which it already produced an alloc failure, so as to not make the same stacktrace fail. Why not store the stack traces themselves in a file? I guess we do not mind (too much) about disk space ? (we now that if we run the same application under e.g. memcheck, the whole set of stacktraces will fit in valgrind memory, so the size should be reasonable). I guess you might then use pub_tool_execontext.h functions to store the stacktraces,and check if a stacktrace is already stored. Also, probably better to have a few regression tests for a tool to be merged. By no way this last comment is a promise that the tool will be merged :). -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 roger.li...@cedalo.com changed: What|Removed |Added Attachment #113820|0 |1 is obsolete|| --- Comment #3 from roger.li...@cedalo.com --- Created attachment 114201 --> https://bugs.kde.org/attachment.cgi?id=114201&action=edit Updated patch with adler32 checksum. Fixes the case where the checksum was being written to the output file incorrectly if the first character was a 0. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #4 from roger.li...@cedalo.com --- Thanks for taking a look. I'm well aware that the set of tools in valgrind is a select bunch, so I'm not expecting a merge. Your understanding of the tool is correct. The idea of using sha3 for storing the call stack is twofold, firstly that is what I used when I was investigating the idea outside of valgrind, and secondly I'm not that familiar with the valgrind api and wanted to present the idea for discussion earlier rather than later. Saving the whole call stack to a file would be much more desirable certainly, it gives you much better visibility over what has gone before / caused a crash than with the current implementation. The part that I'm missing is around loading the call stacks in at the start - could it work in a similar fashion to a suppression file, i.e. the file contents are loaded as the call stacks that we should suppress the allocation failure? I did start to look at regression tests, but didn't want to invest too much time into them before getting a little feedback on the general idea. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290
Ivo Raisr changed:
What|Removed |Added
Ever confirmed|0 |1
Status|UNCONFIRMED |CONFIRMED
CC||iv...@ivosh.net
--- Comment #5 from Ivo Raisr ---
Thank you for sharing this tool with the Valgrind community.
I can see value of this tool in its systemic way in which it fails the
application and taking a central place in an automated solution for checking
memory allocation problems.
The idea of storing call stacks instead of hashes is of course welcome, as it
is much more user friendly and configurable.
Please could you also fix some small problems in your code, such as using
"naked" C types (char), missing space between 'if' and '{'. Also do not include
system files (fcntl.h) - use Valgrind's own ones (VKI_O_RDONLY etc.).
As regards '--depth' command line argument, please could you explain why
existing --num-callers is not suitable to use?
--
You are receiving this mail because:
You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290
--- Comment #6 from roger.li...@cedalo.com ---
> Please could you also fix some small problems in your code, such as using
> "naked" C types (char), missing space between 'if' and '{'. Also do not
> include system files (fcntl.h) - use Valgrind's own ones (VKI_O_RDONLY etc.).
Certainly, that's no problem.
> As regards '--depth' command line argument, please could you explain why
> existing --num-callers is not suitable to use?
The documentation for --num-callers talks about the number of stack being
"shown", I wasn't sure about whether what I was doing was overloading that
meaning in a way that was confusing.
--
You are receiving this mail because:
You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #7 from Ivo Raisr --- Alright, I think it would be preferable to have --num-callers used instead of --depth. Users are already familiar with --num-callers. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 roger.li...@cedalo.com changed: What|Removed |Added Attachment #113910|0 |1 is obsolete|| Attachment #114201|0 |1 is obsolete|| --- Comment #8 from roger.li...@cedalo.com --- Created attachment 114229 --> https://bugs.kde.org/attachment.cgi?id=114229&action=edit Updated patch with fixes and text callstacks. This is an updated patch: * Replaced --depth with standard --num-callers * Call stacks are now saved as text listings * External headers removed * Bare C types replaced with valgrind versions * Formatting fixes I haven't yet added any tests. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #9 from Philippe Waroquiers --- I am wondering also how much difficult it would be to somewhat more generalise the tool. For example, we might want to make similar tests to check failing syscalls. A part of the tool can then be re-used (e.g. storing/loading the stack traces etc ...). We then would need some more command line options to control what to make fail. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #10 from Ivo Raisr --- (In reply to Philippe Waroquiers from comment #9) > I am wondering also how much difficult it would be to somewhat more > generalise the tool. > > For example, we might want to make similar tests to check failing syscalls. Injecting failures in general is a very good topic. However given the imminent Valgrind release, we should decide if this exp-tool can make it for 3.14 in its current shape (and with some regtests available) or not. This decision eventually drives the scope... -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #11 from Philippe Waroquiers --- Yes, I agree that having a way to exercise the error recovery/handling patch of an application is a very good thing to do. IMO, it is very late now to add this, even as an exp tool. A.o. we better discuss the desired functionality somewhat more (e.g. to make a more general/flexible way to make various things fail in a single 'framework/tool'). For big/huge applications, we might need a lot more control about e.g. when to start failing (maybe something like what is available in callgrind: let the tool start doing something once a call to a function has been detected, or once a certain stacktrace has been detected etc ... In summary: the tool as it is now is too minimal and too 'young'. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #12 from Philippe Waroquiers --- ((In reply to Philippe Waroquiers from comment #11) > Yes, I agree that having a way to exercise the error recovery/handling > patch of an application is a very good thing to do. Remove 'patch of' in the above :) -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #13 from Ivo Raisr --- (In reply to roger.light from comment #8) > Created attachment 114229 [details] > Updated patch with fixes and text callstacks. Looks quite good. There are just few nits: - Please remove trailing whitespace you introduced (a few occurrences). For example 'git am' gives the list. - Please have a space between 'while' and the condition. - Please do better job with fixed size buffers (for example in write_callstack_line). Some times ago, Florian underwent a painful exercise auditing all fixed size buffers. - You can use C99 constructs. - Description for command line option --callstack-file deserves a better wording. - Description for command line option --show-fail-traces should be probably renamed to --show-failed-traces (or simply --show-failed). The sentence should start with capital letter anyway. - Fix indentation of function af_instrument. I will post my findings about using tool in a separate reply. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #14 from Ivo Raisr --- The current exp-allocfail crashes badly on my Ubuntu 18.04 box. When running './vg-in-place --tool=exp-allocfail /bin/date', it crashes at af_main.c:119. That's because i is equal to an equivalent of '-1' (4294967295). You need to fix the code at af_main.c:96 - UInt i; + Int i; With this fix in place, I was able to play with ordinary Linux/UNIX standalone programs. Will try to do 'make dist' and check if everything works ok tomorrow. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #15 from Roger Light --- Thanks for the comments and review. I think adding greater capability for controlling where and when failures occur, and adding syscall support could turn this into a really useful tool. I don't think that should take away from there already is though. How about renaming to "failcheck" for example, and rewriting a load of the text to make it clear the tool is about failure checking in general and at the moment considers heap allocation failures, then expanding the scope once you are happy with everything as it stands. Ivo, I'm annoyed I missed the whitespace, I do pay attention to that sort of thing. For af_instrument, you just mean the 4 spaces indentation instead of 3, nothing to do with the function arguments? -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 Roger Light changed: What|Removed |Added Attachment #114229|0 |1 is obsolete|| CC||ro...@atchoo.org --- Comment #16 from Roger Light --- Created attachment 114245 --> https://bugs.kde.org/attachment.cgi?id=114245&action=edit Updated patch with UInt fix, plus others This addresses the points in comments 13 and 14. The reason for the crash is now obvious, I hadn't spotted it because it only manifests on stripped binaries, or if --show-below-main is set. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #17 from Philippe Waroquiers --- (In reply to Roger Light from comment #15) > Thanks for the comments and review. > > I think adding greater capability for controlling where and when failures > occur, and adding syscall support could turn this into a really useful tool. > I don't think that should take away from there already is though. > > How about renaming to "failcheck" for example, and rewriting a load of the > text to make it clear the tool is about failure checking in general and at > the moment considers heap allocation failures, then expanding the scope once > you are happy with everything as it stands. Without some more control on when to start creating failures, the tool will limited to very small applications : big apps quickly have several thousands different alloc stack traces. So, IMO, too early to push in upstream : introducing a new tool has a cost in terms of integrating the patches, and then implies a (forever) cost (maintenance, additional build time, etc ...), and this cost will very probably not be compensated by very usable functionalities. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #18 from Roger Light --- Yes, I see your point. I had this in my mind as more of a tool for working with test suites where you had more control over what was happening anyway, but in reality people will use it as they will. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 roger.li...@cedalo.com changed: What|Removed |Added Attachment #114245|0 |1 is obsolete|| --- Comment #19 from roger.li...@cedalo.com --- Created attachment 114553 --> https://bugs.kde.org/attachment.cgi?id=114553&action=edit Updated patch with better control. This is a work-in-progress update in case there are comments while I'm working on the documentation and tests. * Renamed to failgrind * Added failgrind.h with macros for turning on, turning off, and toggling allocation failures, and clearing the in-memory call stack list. * Added monitor commands to do the same, plus write the current in-memory call stack list to a file, and print or zero stats. * Split --callstack-file into --callstack-input and --callstack-output to allow these to be independent * Added --alloc-threshold-high, --allow-threshold-low, and --alloc-threshold-invert to allow allocations larger/small than a size to always be allowed. * Added --alloc-fail-atstart to allow allocation failures to be disabled on start of failgrind (then to be enabled later on with gdb or --toggle-fn) * Added --toggle-fn that toggles the allocation failure enable/disable state when a names function is both called and later returned from, to allow failgrind testing to be isolated to certain parts of the program. I think that covers most of the relevant control features from callgrind, if you have any comments or suggestions down that road please let me know. I'm least happy that I'm doing everything right in the instrumentation function, so if you did want to take a quick look that would be where I'd appreciate some feedback. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #20 from Ivo Raisr --- Thank you for your work. This is going to be a useful Valgrind tool. I like the documentation for the latest patch. Reasoning explained well on examples. You could mention that the default behaviour (fail unseen allocations) can be heavily tuned by options which are described later. As regards instrumentation: unfortunately I don't think the current scheme of detecting function entry/return w.r.t. --toggle-fn is going to work. Instruction instrumentation is done by blocks and their processing is mostly orthogonal to the actual execution. I think Callgrind tool solves similar problem - detecting when a function is entered and returned - try to have a look there. I think it still suffers from some limitations on arm architecture, though. For coding style - please use explicit check for NULL or != NULL. For example if (instr_callstack) => if (instr_callstack != NULL) -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #21 from roger.li...@cedalo.com --- I'll make the if explicit changes. I still need to have a proper go over the documentation to tie it together as a whole and give more examples of all the options and how they interact - I'll cover the point you mention then. I haven't changed the bulk of what is written there since adding these new controls. Instrumentation certainly takes a bit to get your head around, I don't think I've done that quite yet. I tested the entry/return on a simple example and it worked, but I've just tried something more realistic and you are correct that it fails - it does not detect the return point. I have spent time looking at the callgrind code, but it is involved to say the least. I'll dig into it some more. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #1 from roger.li...@cedalo.com --- Created attachment 113910 --> https://bugs.kde.org/attachment.cgi?id=113910&action=edit Patch using sha3 for hashing instead of adler32 checksums. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 Philippe Waroquiers changed: What|Removed |Added CC||philippe.waroquiers@skynet. ||be --- Comment #2 from Philippe Waroquiers --- I took a (very) quick look at the patch. IIUC, the tool stores a sha3 of the stack traces for which it already produced an alloc failure, so as to not make the same stacktrace fail. Why not store the stack traces themselves in a file? I guess we do not mind (too much) about disk space ? (we now that if we run the same application under e.g. memcheck, the whole set of stacktraces will fit in valgrind memory, so the size should be reasonable). I guess you might then use pub_tool_execontext.h functions to store the stacktraces,and check if a stacktrace is already stored. Also, probably better to have a few regression tests for a tool to be merged. By no way this last comment is a promise that the tool will be merged :). -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 roger.li...@cedalo.com changed: What|Removed |Added Attachment #113820|0 |1 is obsolete|| --- Comment #3 from roger.li...@cedalo.com --- Created attachment 114201 --> https://bugs.kde.org/attachment.cgi?id=114201&action=edit Updated patch with adler32 checksum. Fixes the case where the checksum was being written to the output file incorrectly if the first character was a 0. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #4 from roger.li...@cedalo.com --- Thanks for taking a look. I'm well aware that the set of tools in valgrind is a select bunch, so I'm not expecting a merge. Your understanding of the tool is correct. The idea of using sha3 for storing the call stack is twofold, firstly that is what I used when I was investigating the idea outside of valgrind, and secondly I'm not that familiar with the valgrind api and wanted to present the idea for discussion earlier rather than later. Saving the whole call stack to a file would be much more desirable certainly, it gives you much better visibility over what has gone before / caused a crash than with the current implementation. The part that I'm missing is around loading the call stacks in at the start - could it work in a similar fashion to a suppression file, i.e. the file contents are loaded as the call stacks that we should suppress the allocation failure? I did start to look at regression tests, but didn't want to invest too much time into them before getting a little feedback on the general idea. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290
Ivo Raisr changed:
What|Removed |Added
Ever confirmed|0 |1
Status|UNCONFIRMED |CONFIRMED
CC||iv...@ivosh.net
--- Comment #5 from Ivo Raisr ---
Thank you for sharing this tool with the Valgrind community.
I can see value of this tool in its systemic way in which it fails the
application and taking a central place in an automated solution for checking
memory allocation problems.
The idea of storing call stacks instead of hashes is of course welcome, as it
is much more user friendly and configurable.
Please could you also fix some small problems in your code, such as using
"naked" C types (char), missing space between 'if' and '{'. Also do not include
system files (fcntl.h) - use Valgrind's own ones (VKI_O_RDONLY etc.).
As regards '--depth' command line argument, please could you explain why
existing --num-callers is not suitable to use?
--
You are receiving this mail because:
You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290
--- Comment #6 from roger.li...@cedalo.com ---
> Please could you also fix some small problems in your code, such as using
> "naked" C types (char), missing space between 'if' and '{'. Also do not
> include system files (fcntl.h) - use Valgrind's own ones (VKI_O_RDONLY etc.).
Certainly, that's no problem.
> As regards '--depth' command line argument, please could you explain why
> existing --num-callers is not suitable to use?
The documentation for --num-callers talks about the number of stack being
"shown", I wasn't sure about whether what I was doing was overloading that
meaning in a way that was confusing.
--
You are receiving this mail because:
You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #7 from Ivo Raisr --- Alright, I think it would be preferable to have --num-callers used instead of --depth. Users are already familiar with --num-callers. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 roger.li...@cedalo.com changed: What|Removed |Added Attachment #113910|0 |1 is obsolete|| Attachment #114201|0 |1 is obsolete|| --- Comment #8 from roger.li...@cedalo.com --- Created attachment 114229 --> https://bugs.kde.org/attachment.cgi?id=114229&action=edit Updated patch with fixes and text callstacks. This is an updated patch: * Replaced --depth with standard --num-callers * Call stacks are now saved as text listings * External headers removed * Bare C types replaced with valgrind versions * Formatting fixes I haven't yet added any tests. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #9 from Philippe Waroquiers --- I am wondering also how much difficult it would be to somewhat more generalise the tool. For example, we might want to make similar tests to check failing syscalls. A part of the tool can then be re-used (e.g. storing/loading the stack traces etc ...). We then would need some more command line options to control what to make fail. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #10 from Ivo Raisr --- (In reply to Philippe Waroquiers from comment #9) > I am wondering also how much difficult it would be to somewhat more > generalise the tool. > > For example, we might want to make similar tests to check failing syscalls. Injecting failures in general is a very good topic. However given the imminent Valgrind release, we should decide if this exp-tool can make it for 3.14 in its current shape (and with some regtests available) or not. This decision eventually drives the scope... -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #11 from Philippe Waroquiers --- Yes, I agree that having a way to exercise the error recovery/handling patch of an application is a very good thing to do. IMO, it is very late now to add this, even as an exp tool. A.o. we better discuss the desired functionality somewhat more (e.g. to make a more general/flexible way to make various things fail in a single 'framework/tool'). For big/huge applications, we might need a lot more control about e.g. when to start failing (maybe something like what is available in callgrind: let the tool start doing something once a call to a function has been detected, or once a certain stacktrace has been detected etc ... In summary: the tool as it is now is too minimal and too 'young'. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #12 from Philippe Waroquiers --- ((In reply to Philippe Waroquiers from comment #11) > Yes, I agree that having a way to exercise the error recovery/handling > patch of an application is a very good thing to do. Remove 'patch of' in the above :) -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #13 from Ivo Raisr --- (In reply to roger.light from comment #8) > Created attachment 114229 [details] > Updated patch with fixes and text callstacks. Looks quite good. There are just few nits: - Please remove trailing whitespace you introduced (a few occurrences). For example 'git am' gives the list. - Please have a space between 'while' and the condition. - Please do better job with fixed size buffers (for example in write_callstack_line). Some times ago, Florian underwent a painful exercise auditing all fixed size buffers. - You can use C99 constructs. - Description for command line option --callstack-file deserves a better wording. - Description for command line option --show-fail-traces should be probably renamed to --show-failed-traces (or simply --show-failed). The sentence should start with capital letter anyway. - Fix indentation of function af_instrument. I will post my findings about using tool in a separate reply. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #14 from Ivo Raisr --- The current exp-allocfail crashes badly on my Ubuntu 18.04 box. When running './vg-in-place --tool=exp-allocfail /bin/date', it crashes at af_main.c:119. That's because i is equal to an equivalent of '-1' (4294967295). You need to fix the code at af_main.c:96 - UInt i; + Int i; With this fix in place, I was able to play with ordinary Linux/UNIX standalone programs. Will try to do 'make dist' and check if everything works ok tomorrow. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #15 from Roger Light --- Thanks for the comments and review. I think adding greater capability for controlling where and when failures occur, and adding syscall support could turn this into a really useful tool. I don't think that should take away from there already is though. How about renaming to "failcheck" for example, and rewriting a load of the text to make it clear the tool is about failure checking in general and at the moment considers heap allocation failures, then expanding the scope once you are happy with everything as it stands. Ivo, I'm annoyed I missed the whitespace, I do pay attention to that sort of thing. For af_instrument, you just mean the 4 spaces indentation instead of 3, nothing to do with the function arguments? -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 Roger Light changed: What|Removed |Added Attachment #114229|0 |1 is obsolete|| CC||ro...@atchoo.org --- Comment #16 from Roger Light --- Created attachment 114245 --> https://bugs.kde.org/attachment.cgi?id=114245&action=edit Updated patch with UInt fix, plus others This addresses the points in comments 13 and 14. The reason for the crash is now obvious, I hadn't spotted it because it only manifests on stripped binaries, or if --show-below-main is set. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #17 from Philippe Waroquiers --- (In reply to Roger Light from comment #15) > Thanks for the comments and review. > > I think adding greater capability for controlling where and when failures > occur, and adding syscall support could turn this into a really useful tool. > I don't think that should take away from there already is though. > > How about renaming to "failcheck" for example, and rewriting a load of the > text to make it clear the tool is about failure checking in general and at > the moment considers heap allocation failures, then expanding the scope once > you are happy with everything as it stands. Without some more control on when to start creating failures, the tool will limited to very small applications : big apps quickly have several thousands different alloc stack traces. So, IMO, too early to push in upstream : introducing a new tool has a cost in terms of integrating the patches, and then implies a (forever) cost (maintenance, additional build time, etc ...), and this cost will very probably not be compensated by very usable functionalities. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #18 from Roger Light --- Yes, I see your point. I had this in my mind as more of a tool for working with test suites where you had more control over what was happening anyway, but in reality people will use it as they will. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 roger.li...@cedalo.com changed: What|Removed |Added Attachment #114245|0 |1 is obsolete|| --- Comment #19 from roger.li...@cedalo.com --- Created attachment 114553 --> https://bugs.kde.org/attachment.cgi?id=114553&action=edit Updated patch with better control. This is a work-in-progress update in case there are comments while I'm working on the documentation and tests. * Renamed to failgrind * Added failgrind.h with macros for turning on, turning off, and toggling allocation failures, and clearing the in-memory call stack list. * Added monitor commands to do the same, plus write the current in-memory call stack list to a file, and print or zero stats. * Split --callstack-file into --callstack-input and --callstack-output to allow these to be independent * Added --alloc-threshold-high, --allow-threshold-low, and --alloc-threshold-invert to allow allocations larger/small than a size to always be allowed. * Added --alloc-fail-atstart to allow allocation failures to be disabled on start of failgrind (then to be enabled later on with gdb or --toggle-fn) * Added --toggle-fn that toggles the allocation failure enable/disable state when a names function is both called and later returned from, to allow failgrind testing to be isolated to certain parts of the program. I think that covers most of the relevant control features from callgrind, if you have any comments or suggestions down that road please let me know. I'm least happy that I'm doing everything right in the instrumentation function, so if you did want to take a quick look that would be where I'd appreciate some feedback. -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #20 from Ivo Raisr --- Thank you for your work. This is going to be a useful Valgrind tool. I like the documentation for the latest patch. Reasoning explained well on examples. You could mention that the default behaviour (fail unseen allocations) can be heavily tuned by options which are described later. As regards instrumentation: unfortunately I don't think the current scheme of detecting function entry/return w.r.t. --toggle-fn is going to work. Instruction instrumentation is done by blocks and their processing is mostly orthogonal to the actual execution. I think Callgrind tool solves similar problem - detecting when a function is entered and returned - try to have a look there. I think it still suffers from some limitations on arm architecture, though. For coding style - please use explicit check for NULL or != NULL. For example if (instr_callstack) => if (instr_callstack != NULL) -- You are receiving this mail because: You are watching all bug changes.
[valgrind] [Bug 396290] [PATCH] Possible tool - allocfail
https://bugs.kde.org/show_bug.cgi?id=396290 --- Comment #21 from roger.li...@cedalo.com --- I'll make the if explicit changes. I still need to have a proper go over the documentation to tie it together as a whole and give more examples of all the options and how they interact - I'll cover the point you mention then. I haven't changed the bulk of what is written there since adding these new controls. Instrumentation certainly takes a bit to get your head around, I don't think I've done that quite yet. I tested the entry/return on a simple example and it worked, but I've just tried something more realistic and you are correct that it fails - it does not detect the return point. I have spent time looking at the callgrind code, but it is involved to say the least. I'll dig into it some more. -- You are receiving this mail because: You are watching all bug changes.
