{"action":"created","issue":{"url":"https://api.github.com/repos/dmlc/tvm/issues/2079","repository_url":"https://api.github.com/repos/dmlc/tvm","labels_url":"https://api.github.com/repos/dmlc/tvm/issues/2079/labels{/name}","comments_url":"https://api.github.com/repos/dmlc/tvm/issues/2079/comments","events_url":"https://api.github.com/repos/dmlc/tvm/issues/2079/events","html_url":"https://github.com/dmlc/tvm/pull/2079","id":378758163,"node_id":"MDExOlB1bGxSZXF1ZXN0MjI5Mzg1MjU0","number":2079,"title":"[PASS] InstrumentBoundCheckers pass","user":{"login":"denis0x0D","id":25140014,"node_id":"MDQ6VXNlcjI1MTQwMDE0","avatar_url":"https://avatars2.githubusercontent.com/u/25140014?v=4","gravatar_id":"","url":"https://api.github.com/users/denis0x0D","html_url":"https://github.com/denis0x0D","followers_url":"https://api.github.com/users/denis0x0D/followers","following_url":"https://api.github.com/users/denis0x0D/following{/other_user}","gists_url":"https://api.github.com/users/denis0x0D/gists{/gist_id}","starred_url":"https://api.github.com/users/denis0x0D/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/denis0x0D/subscriptions","organizations_url":"https://api.github.com/users/denis0x0D/orgs","repos_url":"https://api.github.com/users/denis0x0D/repos","events_url":"https://api.github.com/users/denis0x0D/events{/privacy}","received_events_url":"https://api.github.com/users/denis0x0D/received_events","type":"User","site_admin":false},"labels":[{"id":858780915,"node_id":"MDU6TGFiZWw4NTg3ODA5MTU=","url":"https://api.github.com/repos/dmlc/tvm/labels/status:%20need%20review","name":"status: need review","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-11-08T14:34:55Z","updated_at":"2018-11-10T14:47:09Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/dmlc/tvm/pulls/2079","html_url":"https://github.com/dmlc/tvm/pull/2079","diff_url":"https://github.com/dmlc/tvm/pull/2079.diff","patch_url":"https://github.com/dmlc/tvm/pull/2079.patch"},"body":"The patch is related to issue:\r\nhttps://discuss.tvm.ai/t/array-bounds-checking/944\r\n\r\nLets imagine simple example with out of bounds access:\r\n \r\n```\r\nn = tvm.var(\"n\")\r\n \r\n A = tvm.placeholder ((n,n,n), name='A')\r\n B = tvm.placeholder ((n,n,n), name='B')\r\n C = tvm.compute(A.shape, lambda i,j,k: A[i + 300][j][k] + B[i][j][k], name='C')\r\n s = tvm.create_schedule (C.op)\r\n\r\n```\r\n\r\nWe will get IR like this:\r\n```\r\n produce C {\r\n    for (i, 0, n) {\r\n      for (j, 0, n) {\r\n        for (k, 0, n) {\r\n          C[((((i*n) + j)*n) + k)] = (A[(((((i + 300)*n) + j)*n) + k)] + B[((((i*n) + j)*n) + k)])\r\n        }\r\n      }\r\n    }\r\n  }\r\n```\r\nThis is actually out of the bounds access, this code could cause a segfault or access to \"junk\" memory since TMV has native runtime.\r\n\r\nCould we set for example, in debug mode, an instrumentation before every memory access with simple check and assert.  So at the runtime we can handle it, and make sure our model does not access any invalid memory.\r\n\r\n ```\r\nproduce C {\r\n    for (i, 0, n) {\r\n      for (j, 0, n) {\r\n        for (k, 0, n) {\r\n          if (((((((((i + 300)*n) + j)*n) + k) < ((n*n)*n)) && (((((i*n) + j)*n) + k) < ((n*n)*n))) && (((((i*n) + j)*n) + k) < ((n*n)*n)))) {\r\n            C[((((i*n) + j)*n) + k)] = (A[(((((i + 300)*n) + j)*n) + k)] + B[((((i*n) + j)*n) + k)])\r\n          } else {\r\n            assert(((((((((i + 300)*n) + j)*n) + k) < ((n*n)*n)) && (((((i*n) + j)*n) + k) < ((n*n)*n))) && (((((i*n) + j)*n) + k) < ((n*n)*n))), \"OUT OF BOUNDS\")\r\n            1\r\n          }\r\n        }\r\n      }\r\n    }\r\n  }\r\n```\r\n\r\nSo, I wrote the pass, which instruments bound check before potentially invalid memory access.\r\n\r\nThe pass could be enabled by option **instrument_bound_checkers=True**.\r\nThe idea is simple,\r\nat first we associate every buffer_var with actual Buffer shape, as far as I understood, in term of TVM buffer_var is like a pointer to actual Buffer, and then in special pass insert check before memory access.\r\nAlso new buffer could be created - we check it.\r\nSome buffers could increase size by allocate - we check it.\r\nCould have situation when optimization pass insert an intrinsic tvm.if_then_else - we check it.\r\n\r\nI think it could solve the problem mentioned in the issue. \r\n\r\nCould someone please review and give me any feedback.\r\nThanks.\r\n"},"comment":{"url":"https://api.github.com/repos/dmlc/tvm/issues/comments/437588831","html_url":"https://github.com/dmlc/tvm/pull/2079#issuecomment-437588831","issue_url":"https://api.github.com/repos/dmlc/tvm/issues/2079","id":437588831,"node_id":"MDEyOklzc3VlQ29tbWVudDQzNzU4ODgzMQ==","user":{"login":"denis0x0D","id":25140014,"node_id":"MDQ6VXNlcjI1MTQwMDE0","avatar_url":"https://avatars2.githubusercontent.com/u/25140014?v=4","gravatar_id":"","url":"https://api.github.com/users/denis0x0D","html_url":"https://github.com/denis0x0D","followers_url":"https://api.github.com/users/denis0x0D/followers","following_url":"https://api.github.com/users/denis0x0D/following{/other_user}","gists_url":"https://api.github.com/users/denis0x0D/gists{/gist_id}","starred_url":"https://api.github.com/users/denis0x0D/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/denis0x0D/subscriptions","organizations_url":"https://api.github.com/users/denis0x0D/orgs","repos_url":"https://api.github.com/users/denis0x0D/repos","events_url":"https://api.github.com/users/denis0x0D/events{/privacy}","received_events_url":"https://api.github.com/users/denis0x0D/received_events","type":"User","site_admin":false},"created_at":"2018-11-10T14:47:09Z","updated_at":"2018-11-10T14:47:09Z","author_association":"NONE","body":"@ajtulloch thanks for point this out.\r\nAt first I was thinking about to just add ASan's pass to backend,\r\nbut IMHO we should be really careful with it, because it could not be a generic solution:\r\n1. ASan alongside with instrumentation pass requires runtime part, which allocates shadow memory and manages it like poison/unpoison and so on. The place of the shadow memory depends on your OS and architecture, we can face really hard bugs with it.\r\nI personally faced some issues:\r\na. ASan's runtime was not initialized before instrumented binary/library - which cause a segfault.\r\nb. GDB maps binary with PIE to ASan's shadow on 32 bit ARM Tizen OS.\r\nc. Broken \"fast unwinder\" on 32 bit thumb https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00312.html?fbclid=IwAR0hP_a3kFdCZbwMq07kjQnhoPdfg_3MZZya9roPYy8l8I46rpZjgj3TMM4\r\nAnd so.\r\n2. ASan officially does support 32 bit and 64 bit ARM only for Android, so other OS requires to have patched version of ASan's runtime.\r\n3. GPU support? Depends on how it allocates memory, some issue https://devtalk.nvidia.com/default/topic/1037466/cuda-programming-and-performance/cuda-runtime-library-and-addresssanitizer-incompatibilty/\r\n\r\nSo, IMHO that every architecture which TVM supports and every new architecture would require to custom ASan's runtime, but otherwise if we need just x86/x64 it should be ok.\r\nThanks."}}
{"actor":{"display_login":"denis0x0D"}}