)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1000716,"name":"Christopher Head","email":"chead@zaber.com","username":"Hawk777"},"change_message_id":"8eddc30aaec19fa072cc18d259a97eb7c72f58ba","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"b58c0313_8092518e","updated":"2025-05-21 02:14:10.000000000","message":"Just curious, this seems more complicated than just using sig_atomic_t, which ISO says should be sufficient for this purpose; is there a benefit to looking up a lockless atomic_int type instead?","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"cc751917389cef862cd5e4665885e9f2f17fd649","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"ec9d5ab6_9d281584","updated":"2025-05-20 09:58:26.000000000","message":"Some context for the change.\n\nIn our LAB environment we observe that under very rare circumstances and under heavy load openocd does not response to SIGINT in an expected manner. Meaning, we send SIGINT via `kill` utility and do not observe a response in the log file. The issue does not happen very often (I would say around once in 2000 launches), and we have several instances running in parallel. This situation motivated me to investigate if there are any issues related to signal processing on OpenOCD side. \n\nLooks like I\u0027ve managed to find some potential issues (see the description of this commit). Fixing/investigating my original problem is still in progress - it is still possible that the root cause for my original problem is in the test driver we use. However, I believe the changes from this PR could be useful.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"81ce93586b20b97360d7c9b011324ccfa91f959a","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"716c2464_37aefade","in_reply_to":"14ce9f9f_e8b2fefe","updated":"2025-05-23 07:16:48.000000000","message":"@Christopher I\u0027ve double-checked and yes, you are correct. `sig_atomic_t` should be enough. Addressed.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"97b04791afcac78f5e15db5144e3b07d7f6171c2","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"ab867224_03d2f39d","in_reply_to":"8fb3ea40_497102cc","updated":"2025-05-20 13:22:15.000000000","message":"\u003e When you say that there is one case over 2000 that fails to quit, you mean using exactly the same OpenOCD binary, or OpenOCD gets recompiled between one test and the next one?\n\nYes, exactly the same binary. It\u0027s just we run several OpenOCD instances in parallel and sometimes we observe the issue. The number \"2000\" is an approximate number to get the scale of how often this happens. \n\nAgain, this may be the fact that we use signal-unsafe functions for debug printing. By the way, our testing runs with \"-d3\" option. Or as mentioned in my original \ncomment this still can be an issue with our driver.\n\n___\n\u003e Have you tried to run OpenOCD under valgrind to see if it detects some incorrect memory access?\n\nYes, we do regularly run our testsuite with valgrind and UB sanitizer. Both report nothing in this case.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"0f1545f9de302ef104b1130fb4a3f2f833fce538","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"14ce9f9f_e8b2fefe","in_reply_to":"b58c0313_8092518e","updated":"2025-05-21 03:03:10.000000000","message":"@Christopher Head I need to double check, but it seems that `sig_atomic_t` should be enough, actually. Somehow I\u0027ve missed that. This was also suggested by Antonio in his latest comment.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"e0f60fabea810dd5c856f6114e5ca515a942abea","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":2,"id":"8fb3ea40_497102cc","in_reply_to":"ec9d5ab6_9d281584","updated":"2025-05-20 12:17:05.000000000","message":"Hi Anatoly,\nthanks for the patch and for rising the discussion.\nWhen you say that there is one case over 2000 that fails to quit, you mean using exactly the same OpenOCD binary, or OpenOCD gets recompiled between one test and the next one?\n\nI\u0027m checking the code to understand why you gets such issue, but I cannot detect anything.\nThere is a strange assignment\n```\nsrc/server/server.c:599:\n\treturn shutdown_openocd \u003d\u003d SHUTDOWN_WITH_ERROR_CODE ? ERROR_FAIL : ERROR_OK;\n```\nwhere the value `ERROR_OK \u003d\u003d 0` clashes with `CONTINUE_MAIN_LOOP \u003d\u003d 0` too.\nBut this is already at the exit of the main loop, so not really able to produce any issue.\n\nThen, clearly we need to have `shutdown_openocd` declared as `volatile`, but I don\u0027t see how this can impact only one every 2000 executions, mainly if the same binary has been used for all the 2000 runs.\n\nHave you tried to run OpenOCD under valgrind to see if it detects some incorrect memory access?","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"}],"src/server/server.c":[{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"e0f60fabea810dd5c856f6114e5ca515a942abea","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"1b6c0547_ab65eab7","line":56,"updated":"2025-05-20 12:17:05.000000000","message":"The variable `shutdown_openocd` is set in the signal handler, while the rest of the code is not aware that it\u0027s potentially changed.\nA loop that continuously tests this variable could read it only once and reuse the old value, never noticing that the value is changed.\nSo having `shutdown_openocd` as `volatile` it\u0027s definitively correct!\n\nBut why you want it to be `atomic` ?\nThere is no read-modify-write involved, and you do not use the API defined in `stdatomic.h` to modify it atomically.\nIt looks incorrect or over-killing to me.\n\nInstead, I believe here we could improve readability by splitting the variable `shutdown_openocd` in:\n```\nvolatile bool shutdown_async_by_signal;\nbool shutdown_sync_by_sw;\nint shutdown_error_code;\n```","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"97b04791afcac78f5e15db5144e3b07d7f6171c2","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"2fc6ca90_89205d94","line":56,"in_reply_to":"1b6c0547_ab65eab7","updated":"2025-05-20 13:22:15.000000000","message":"\u003e But why you want it to be atomic ?\n\nApparently, (to my surprise) C language standard requires it. Again, it just happened that I had an opportunity to dig into this, otherwise I would\u0027ve never figured that out. Here is the relevant part. I use cppreference for convince (https://en.cppreference.com/w/c/program/signal) , I can quote the C11 or C23 if really needed.\n\n\n**Part 1.**\n\n```\nIf the signal handler is called NOT as a result of abort\nor raise (in other words, the signal handler is asynchronous),\nthe behavior is undefined if\n...\nthe signal handler refers to any object with static or\nthread-local(since C11) storage duration that is not a\nlock-free atomic(since C11) other than by assigning to\na static volatile sig_atomic_t.\n```\n\n**Part 2.**\n\n```\nOn entry to the signal handler, the state of the floating-point\nenvironment and the values of all objects is unspecified,\n except for\n\nobjects of type volatile sig_atomic_t objects of lock-free\natomic types (since C11) side effects made visible through\natomic_signal_fence (since C11)\nOn return from a signal handler, the value of any object\nmodified by the signal handler that is not volatile sig_atomic_t\nor lock-free atomic(since C11) is undefined.\n```\n\n___\nRegarding this one:\n\n\u003e here is no read-modify-write involved, ... It looks incorrect or over-killing to me.\n\n\nI don\u0027t use those read-modify-write with relaxed memory ordering because I don\u0027t think we really need those, since in our case we are concerned only about correctness of the code, not the performance. In addition I\u0027m just scared :)\n\nIn this example I indeed \"over killing\" it (from the description of _Atomic https://en.cppreference.com/w/c/language/atomic)\n\n```\nBuilt-in increment and decrement operators and compound assignment\nare read-modify-write atomic operations with total sequentially\nconsistent ordering (as if using memory_order_seq_cst). \n```\n\nHowever, I think this is justified, given the nature and the context of these operations. The performance impact should be negligible.\n\n___\n\u003e Instead, I believe here we could improve readability by splitting the variable shutdown_openocd in:\n\nI\u0027ll try to revise this code, thanks. In the meantime it would be great if we could sort-out the atomicy/lock-free requirements.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"a4f78b34c4461f73e23d2e753539d50546db5d42","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"a491abb5_90d8a17a","line":56,"in_reply_to":"2fc6ca90_89205d94","updated":"2025-05-20 14:55:18.000000000","message":"On which host OS you get the issue?\n\nIn the link you reference above, I also read:\n\"POSIX recommends sigaction instead of signal, due to its underspecified behavior and significant implementation variations, regarding signal delivery while a signal handler is executed.\"\nand there is a link to sigaction()\nhttps://pubs.opengroup.org/onlinepubs/9699919799/functions/sigaction.html\nbut sigaction() is not present in mingw, so we cannot compile it for Windows.\nLet\u0027s stick at signal().\n\nThe same link reports:\n\"On return from a signal handler, the value of any object modified by the signal handler that is not volatile sig_atomic_t or lock-free atomic(since C11) is undefined.\"\nIn https://en.cppreference.com/w/c/program/sig_atomic_t\nI see a simpler implementation with sig_atomic_t that does not require querying ATOMIC_INT_LOCK_FREE and does not use `atomic_int`.\n\nI would prefer a solution that splits the variable `shutdown_openocd` as I wrote above, so the signal handler is the only one to modify the variable and the normal code only reads it. Using `volatile sig_atomic_t` instead of `volatile atomic_int`.\nThen, drop the `typedef` as we cannot have new ones in OpenOCD.\n\nBut the real decision maker is your failing test!","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"81ce93586b20b97360d7c9b011324ccfa91f959a","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"ba2dd550_b03272d8","line":56,"in_reply_to":"3cc5d6a9_6a277209","updated":"2025-05-23 07:16:48.000000000","message":"@Antonio:\n\n\u003e I see a simpler implementation with sig_atomic_t that does not require querying ATOMIC_INT_LOCK_FREE and does not use atomic_int.\n\nAddressed. Just replaced the respected variables to `sig_atomic_t`.\n\n___\n\n\n\u003e I would prefer a solution that splits the variable shutdown_openocd as I wrote above, so the signal handler is the only one to modify the variable and the normal code only reads it. Using volatile sig_atomic_t instead of volatile atomic_int.\n\nI\u0027ve addressed it but in a separate MR. https://review.openocd.org/c/openocd/+/8932/1 . The reason is that the amount of changes required, so I feel like it will be better to review it separately.\n\nI\u0027ve also ended up with 4 globals instead of 3, since I don\u0027t want to merge sync and async state, so I track async error and sync error separately.\n\n\n___\n\nQuote from Antonio:\n\n\u003e But the real decision maker is your failing test!\n\nQuote from me:\n\n\u003e I think I have an idea of how to proceed with debugging that.\n\nI\u0027ve done some additional testing. Result are:\n\n1. Applying this patch **alone** does not fix my issue.\n2. However, if calls to LOG_DEBUG inside the signal handler are removed, then I don\u0027t observe the issue anymore. So it may be possible that the issue I observe is caused by bullets \"3\" and \"4\" from the description of this MR (usage of signal-unsafe functions inside signal handler). I\u0027ll try to address this in a follow-up commit.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"f408b114749971bb66031bed79c0bbc22f161908","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"f4842153_5c81a122","line":56,"in_reply_to":"4cbc0ce6_7b111a05","updated":"2025-05-25 19:06:54.000000000","message":"\u003e ... make these signal handlers hyper-slim ... The logs could be moved in the main loop when we detect that the signal has arrived.\n\n\nMy concern is the following:\n\nAt the present moment OpenOCD has no facilities to cancel long-standing operations. For example, if we load 40mb file (like Linux Kernel image) it can take    something around 20-30 minutes, depending on the adapter speed and target HW. \n\nIn my experience if user wants to interrupt such operation, he/she sends a bunch of ctrl+C and then just kills the process with `kill -9`. My personal preference would be to observe the SIGINT corresponding to the user input before the process is terminated by OS.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"8b3ce75cf504fd1efa0376e2c1e5027e918009fe","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"e8bef644_1134c188","line":56,"in_reply_to":"9e585dc2_b20f2e7b","updated":"2025-05-23 17:53:08.000000000","message":"@Antonio:\n\n\u003e I think that such LOG_DEBUG() should be removed from the handlers in this file.\n\nYes, it certainly does. It\u0027s just I wanted to do that in another commit. I it is useful to print information about the signal in the log. We can do that via a raw `write` call. The write is signal-safe. We don\u0027t even need the formatted output that match. The reason I don\u0027t do this in this MR is that there will be some extra effort to untangle how LOG_DEBUG works internally and which descriptors I can pass to the `write` call. Is it appropriate or do you insist on addressing this as part of **this** MR?","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"0f1545f9de302ef104b1130fb4a3f2f833fce538","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"3cc5d6a9_6a277209","line":56,"in_reply_to":"a491abb5_90d8a17a","updated":"2025-05-21 03:03:10.000000000","message":"\u003e On which host OS you get the issue?\n\nLinux/ubuntu 22.\n\n___\n\n\u003e ATOMIC_INT_LOCK_FREE and does not use atomic_int.\n\nyes, it seems that it should be sufficient. I\u0027ll adjust the PR to use those.\n\n___\n\u003e I would prefer a solution that splits the variable shutdown_openocd as I wrote above, so the signal handler is the only one to modify the variable and the normal code only reads it. Using volatile sig_atomic_t instead of volatile atomic_int.\n\nI\u0027ll try do that.\n\n___\n\u003e But the real decision maker is your failing test!\n\nAs for my test goes... I think I have an idea of how to proceed with debugging that.\n1. I\u0027ll need to adopt some variant of this patch (mostly to add volatile statements, since I\u0027m on x86). Had no chance to do the stress-testing with this patch adopted, but I\u0027ll experiment before producing the next iteration of the patch.\n2. If the issue persist, I can remove LOG_DEBUG statements from signal handler and see if it helps. So I should be able to provide (with relatively high degree of certainty) to conclude if it\u0027s OpenOCD codebase or the problem in test driver I use.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"1e24c00ca45bb79a2280ebdfa523b112af8bd124","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"9e585dc2_b20f2e7b","line":56,"in_reply_to":"ba2dd550_b03272d8","updated":"2025-05-23 13:31:40.000000000","message":"Issue fixed removing the `LOG_DEBUG()`!\nThis is in line with limiting the scope of the signal handler.\nAnd `LOG_DEBUG()` surely modifies something that is not `sig_atomic_t`.\nI think that such `LOG_DEBUG()` should be removed from the handlers in this file.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"e52e9cd493bfc75aed16c7d49e8c344abea2f54f","unresolved":true,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"4cbc0ce6_7b111a05","line":56,"in_reply_to":"e8bef644_1134c188","updated":"2025-05-24 15:38:46.000000000","message":"You can use another patch, if you prefer, but to fix your issue I think we can make these signal handlers hyper-slim, just writing the variable `static volatile sig_atomic_t` and nothing more.\nThe logs could be moved in the main loop when we detect that the signal has arrived.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"549c0d1ddbe291d8f0ef21562cb747997477ab9f","unresolved":false,"context_lines":[{"line_number":53,"context_line":"typedef volatile int signal_updated_int_t;"},{"line_number":54,"context_line":"#endif"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-csrc","patch_set":2,"id":"ed804739_64ec5e38","line":56,"in_reply_to":"f4842153_5c81a122","updated":"2025-08-17 10:51:37.000000000","message":"That\u0027s true. Long operation are not interrupted. And Tcl loops error like\n`for {set i 0} {$i \u003c 10} {} {}`\nnever quit.\nOk. To be reviewed if we get a way to interrupt long loop","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"e0f60fabea810dd5c856f6114e5ca515a942abea","unresolved":true,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"/* set the polling period to 100ms */"},{"line_number":61,"context_line":"static int polling_period \u003d 100;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"4855ba61_c812cb49","line":58,"updated":"2025-05-20 12:17:05.000000000","message":"From the code I don\u0027t see the reason for changing it. It\u0027s value is only read at exit.\nAnyway, since it is changed by a signal handler, I\u0027m ok at having it as `volatile` too.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"a4f78b34c4461f73e23d2e753539d50546db5d42","unresolved":false,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"/* set the polling period to 100ms */"},{"line_number":61,"context_line":"static int polling_period \u003d 100;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"009e7c27_3d567653","line":58,"in_reply_to":"25cf111c_3af64148","updated":"2025-05-20 14:55:18.000000000","message":"Ack","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"97b04791afcac78f5e15db5144e3b07d7f6171c2","unresolved":true,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"static signal_updated_int_t shutdown_openocd \u003d CONTINUE_MAIN_LOOP;"},{"line_number":57,"context_line":"/* store received signal to exit application by killing ourselves */"},{"line_number":58,"context_line":"static signal_updated_int_t last_signal;"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"/* set the polling period to 100ms */"},{"line_number":61,"context_line":"static int polling_period \u003d 100;"}],"source_content_type":"text/x-csrc","patch_set":2,"id":"25cf111c_3af64148","line":58,"in_reply_to":"4855ba61_c812cb49","updated":"2025-05-20 13:22:15.000000000","message":"\u003e From the code I don\u0027t see the reason for changing it. It\u0027s value is only read at exit. \nAnyway, since it is changed by a signal handler, I\u0027m ok at having it as volatile too\n\nWell, it\u0027s just to strictly comply with the standard. I\u0027ve quoted the interesting parts above. Could you please consider revise this?\n\nIn my case x86 access are most likely atomic and lock-free, however on other platforms theoretically this may be not the case.","commit_id":"bb22c49e65c9338bd6fd0977388e6bdc610923aa"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"1e24c00ca45bb79a2280ebdfa523b112af8bd124","unresolved":true,"context_lines":[{"line_number":604,"context_line":"\t/* store only first signal that hits us */"},{"line_number":605,"context_line":"\tif (shutdown_openocd \u003d\u003d CONTINUE_MAIN_LOOP) {"},{"line_number":606,"context_line":"\t\tshutdown_openocd \u003d SHUTDOWN_WITH_SIGNAL_CODE;"},{"line_number":607,"context_line":"\t\tassert(sig \u003e\u003d SIG_ATOMIC_MIN \u0026\u0026 sig \u003c\u003d SIG_ATOMIC_MAX);"},{"line_number":608,"context_line":"\t\tlast_signal \u003d sig;"},{"line_number":609,"context_line":"\t\tLOG_DEBUG(\"Terminating on Signal %d\", sig);"},{"line_number":610,"context_line":"\t} else"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"1d6831bf_56528b02","line":607,"updated":"2025-05-23 13:31:40.000000000","message":"As you wrote in the commit message,\n```\n3. Signal handlers may only call a very limited subset of standard\n   library functions.\n```\nand https://en.cppreference.com/w/c/program/signal say\n```\n  - the signal handler calls any function within the standard library, except \n      - abort\n      - _Exit\n      - quick_exit\n      - signal with the first argument being the number of the signal currently\n        handled (async handler can re-register itself, but not other signals).\n      - atomic functions from \u003cstdatomic.h\u003e if the atomic arguments are lock-free\n      - atomic_is_lock_free (with any kind of atomic arguments)\n```\nI have checked what\u0027s behind `assert()` in gnu libc (becase this function does not always returns) and I found it calls `abort()` listed above!\n\nI think we can simply drop this `assert()` knowing that `sig` is of type `int` and also `sig_atomic_t` is a `typedef` of the same type.","commit_id":"73d7a12534e62c8ccb055965aee99b046c2578f8"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"8b3ce75cf504fd1efa0376e2c1e5027e918009fe","unresolved":true,"context_lines":[{"line_number":604,"context_line":"\t/* store only first signal that hits us */"},{"line_number":605,"context_line":"\tif (shutdown_openocd \u003d\u003d CONTINUE_MAIN_LOOP) {"},{"line_number":606,"context_line":"\t\tshutdown_openocd \u003d SHUTDOWN_WITH_SIGNAL_CODE;"},{"line_number":607,"context_line":"\t\tassert(sig \u003e\u003d SIG_ATOMIC_MIN \u0026\u0026 sig \u003c\u003d SIG_ATOMIC_MAX);"},{"line_number":608,"context_line":"\t\tlast_signal \u003d sig;"},{"line_number":609,"context_line":"\t\tLOG_DEBUG(\"Terminating on Signal %d\", sig);"},{"line_number":610,"context_line":"\t} else"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"50699431_abb9ada0","line":607,"in_reply_to":"1d6831bf_56528b02","updated":"2025-05-23 17:53:08.000000000","message":"\u003e the signal handler calls any function within the standard library, except \n\nA more complete quote is:\n\n\u003e the behavior is undefined if the signal handler calls any function within the standard library, except \n\nI think this line explicitly allows use `abort`.\n\n\u003e I think we can simply drop this assert() knowing that sig is of type int and also sig_atomic_t is a typedef of the same type.\n\nThe thing is that we don\u0027t know that. `sig_atomic_t` is not guaranteed to be `int`. The underlying type is implementation-defined AFAIK. The intention is to check if our platform can represent signal number (which is platform-dependent) in a `sig_atomic_t` which may not be true on some exotic system. I can certainly remove it - no big deal, however, could you revise this once again?","commit_id":"73d7a12534e62c8ccb055965aee99b046c2578f8"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"9719fae214bfe14b7182808f8888cd1c8d9b4ed7","unresolved":true,"context_lines":[{"line_number":604,"context_line":"\t/* store only first signal that hits us */"},{"line_number":605,"context_line":"\tif (shutdown_openocd \u003d\u003d CONTINUE_MAIN_LOOP) {"},{"line_number":606,"context_line":"\t\tshutdown_openocd \u003d SHUTDOWN_WITH_SIGNAL_CODE;"},{"line_number":607,"context_line":"\t\tassert(sig \u003e\u003d SIG_ATOMIC_MIN \u0026\u0026 sig \u003c\u003d SIG_ATOMIC_MAX);"},{"line_number":608,"context_line":"\t\tlast_signal \u003d sig;"},{"line_number":609,"context_line":"\t\tLOG_DEBUG(\"Terminating on Signal %d\", sig);"},{"line_number":610,"context_line":"\t} else"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"75d85ed9_f8d1fba3","line":607,"in_reply_to":"4ede3220_13ade763","updated":"2025-08-16 05:50:39.000000000","message":"@Antonio, can we revive this MR? Could you take a look once again? Should I remove the assert?","commit_id":"73d7a12534e62c8ccb055965aee99b046c2578f8"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"e52e9cd493bfc75aed16c7d49e8c344abea2f54f","unresolved":true,"context_lines":[{"line_number":604,"context_line":"\t/* store only first signal that hits us */"},{"line_number":605,"context_line":"\tif (shutdown_openocd \u003d\u003d CONTINUE_MAIN_LOOP) {"},{"line_number":606,"context_line":"\t\tshutdown_openocd \u003d SHUTDOWN_WITH_SIGNAL_CODE;"},{"line_number":607,"context_line":"\t\tassert(sig \u003e\u003d SIG_ATOMIC_MIN \u0026\u0026 sig \u003c\u003d SIG_ATOMIC_MAX);"},{"line_number":608,"context_line":"\t\tlast_signal \u003d sig;"},{"line_number":609,"context_line":"\t\tLOG_DEBUG(\"Terminating on Signal %d\", sig);"},{"line_number":610,"context_line":"\t} else"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"cb4eba3c_ed303762","line":607,"in_reply_to":"50699431_abb9ada0","updated":"2025-05-24 15:38:46.000000000","message":"Yes, reading better I agree, `abort()` can be used.\nC99 and following report that\nhttp://port70.net/~nsz/c/c99/n1256.html#7.18.3\n\"If sig_atomic_t ... is defined as a signed integer type, the value of SIG_ATOMIC_MIN shall be no greater than -127 and the value of SIG_ATOMIC_MAX shall be no less than 127; otherwise, sig_atomic_t is defined as an unsigned integer type, and the value of SIG_ATOMIC_MIN shall be 0 and the value of SIG_ATOMIC_MAX shall be no less than 255.\"\n\nSince the signals are in the range 1~63, I still consider the `assert()` as not needed.","commit_id":"73d7a12534e62c8ccb055965aee99b046c2578f8"},{"author":{"_account_id":1000021,"name":"Antonio Borneo","email":"borneo.antonio@gmail.com","username":"borneoa"},"change_message_id":"549c0d1ddbe291d8f0ef21562cb747997477ab9f","unresolved":false,"context_lines":[{"line_number":604,"context_line":"\t/* store only first signal that hits us */"},{"line_number":605,"context_line":"\tif (shutdown_openocd \u003d\u003d CONTINUE_MAIN_LOOP) {"},{"line_number":606,"context_line":"\t\tshutdown_openocd \u003d SHUTDOWN_WITH_SIGNAL_CODE;"},{"line_number":607,"context_line":"\t\tassert(sig \u003e\u003d SIG_ATOMIC_MIN \u0026\u0026 sig \u003c\u003d SIG_ATOMIC_MAX);"},{"line_number":608,"context_line":"\t\tlast_signal \u003d sig;"},{"line_number":609,"context_line":"\t\tLOG_DEBUG(\"Terminating on Signal %d\", sig);"},{"line_number":610,"context_line":"\t} else"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"2ad53342_a2369a9e","line":607,"in_reply_to":"75d85ed9_f8d1fba3","updated":"2025-08-17 10:51:37.000000000","message":"I still don\u0027t feel we should not use an assert in this case, but I want this to be part of the v1.0.0 to get feedback on all build platforms\nLet\u0027s merge it as it","commit_id":"73d7a12534e62c8ccb055965aee99b046c2578f8"},{"author":{"_account_id":1002161,"name":"Anatoly P","email":"kupokupokupopo@gmail.com","username":"ecco_the_dolphin"},"change_message_id":"f408b114749971bb66031bed79c0bbc22f161908","unresolved":true,"context_lines":[{"line_number":604,"context_line":"\t/* store only first signal that hits us */"},{"line_number":605,"context_line":"\tif (shutdown_openocd \u003d\u003d CONTINUE_MAIN_LOOP) {"},{"line_number":606,"context_line":"\t\tshutdown_openocd \u003d SHUTDOWN_WITH_SIGNAL_CODE;"},{"line_number":607,"context_line":"\t\tassert(sig \u003e\u003d SIG_ATOMIC_MIN \u0026\u0026 sig \u003c\u003d SIG_ATOMIC_MAX);"},{"line_number":608,"context_line":"\t\tlast_signal \u003d sig;"},{"line_number":609,"context_line":"\t\tLOG_DEBUG(\"Terminating on Signal %d\", sig);"},{"line_number":610,"context_line":"\t} else"}],"source_content_type":"text/x-csrc","patch_set":4,"id":"4ede3220_13ade763","line":607,"in_reply_to":"cb4eba3c_ed303762","updated":"2025-05-25 19:06:54.000000000","message":"\u003e Since the signals are in the range 1~63, I still consider the assert() as not needed.\n\nSorry for being pedantic, but given the context of this discussion, I hope it\u0027s not an issue :) I believe the quoted statement holds true for Unix and Windows systems.\n\nHowever, my understanding is that neither POSIX not C standard doesn\u0027t actually require specific numerical values for any of these signals. Here\u0027s what I found in the freely available POSIX standard draft (using this link: https://pubs.opengroup.org/onlinepubs/9699919799/ , searched for signal.h):\n\n\u003e The \u003csignal.h\u003e header shall define the following macros that are used to refer to the signals that occur in the system. Signals defined here begin with the letters SIG followed by an uppercase letter. The macros shall expand to positive integer constant expressions with type int and distinct values. The value 0 is reserved for use as the null signal (see kill()). Additional implementation-defined signals may occur in the system.\n\nIt does not look like POSIX requires a specific numbering scheme for these signals.\n\nMy understanding is that OpenOCD attempts to maintain compatibility even with exotic systems (I recall seeing comments in the types.h header suggesting OpenOCD could be built for eCos, for instance). I\u0027m not sure if this is still the case, but if it is - on such systems, the values of these signal constants might indeed differ (admittedly, when I checked the eCos source code, I found their signal implementation uses numbers from 1-17).\n\nDoes this motivation makes sense? Or is it an unnecessary pedantry/overkill ?","commit_id":"73d7a12534e62c8ccb055965aee99b046c2578f8"}]}