Merge pull request #1538 from Smit-create/i-1536

Smit-create · web-flow · commit 8bd6143a99b2 · 2023-02-21T22:49:41.000+05:30
ASR/LLVM: Fix issues with string item
diff --git a/src/libasr/codegen/asr_to_c.cpp b/src/libasr/codegen/asr_to_c.cpp
@@ -1211,8 +1211,7 @@ R"(
         std::string idx = std::move(src);
         this->visit_expr(*x.m_arg);
         std::string str = std::move(src);
-        src = "(char *)memcpy((char *)calloc(2U, sizeof(char)), "
-                + str + " + " + idx + " - 1, 1U)";
+        src = "_lfortran_str_item(" + str + ", " + idx + ")";
     }
 
     void visit_StringLen(const ASR::StringLen_t &x) {
diff --git a/src/libasr/codegen/asr_to_llvm.cpp b/src/libasr/codegen/asr_to_llvm.cpp
@@ -1060,19 +1060,19 @@ class ASRToLLVMVisitor : public ASR::BaseVisitor<ASRToLLVMVisitor>
         return builder->CreateCall(fn, {str});
     }
 
-    llvm::Value* lfortran_str_copy(llvm::Value* str, llvm::Value* idx1, llvm::Value* idx2)
+    llvm::Value* lfortran_str_item(llvm::Value* str, llvm::Value* idx1)
     {
-        std::string runtime_func_name = "_lfortran_str_copy";
+        std::string runtime_func_name = "_lfortran_str_item";
         llvm::Function *fn = module->getFunction(runtime_func_name);
         if (!fn) {
             llvm::FunctionType *function_type = llvm::FunctionType::get(
                     character_type, {
-                        character_type, llvm::Type::getInt32Ty(context), llvm::Type::getInt32Ty(context)
+                        character_type, llvm::Type::getInt32Ty(context)
                     }, false);
             fn = llvm::Function::Create(function_type,
                     llvm::Function::ExternalLinkage, runtime_func_name, *module);
         }
-        return builder->CreateCall(fn, {str, idx1, idx2});
+        return builder->CreateCall(fn, {str, idx1});
     }
 
     llvm::Value* lfortran_str_slice(llvm::Value* str, llvm::Value* idx1, llvm::Value* idx2,
@@ -1907,7 +1907,7 @@ class ASRToLLVMVisitor : public ASR::BaseVisitor<ASRToLLVMVisitor>
                 std::vector<llvm::Value*> idx_vec = {idx};
                 p = CreateGEP(str, idx_vec);
             } else {
-                p = lfortran_str_copy(str, idx, idx);
+                p = lfortran_str_item(str, idx);
             }
             // TODO: Currently the string starts at the right location, but goes to the end of the original string.
             // We have to allocate a new string, copy it and add null termination.
@@ -1990,7 +1990,9 @@ class ASRToLLVMVisitor : public ASR::BaseVisitor<ASRToLLVMVisitor>
         // llvm::Value *p = CreateGEP(str, idx_vec);
         // TODO: Currently the string starts at the right location, but goes to the end of the original string.
         // We have to allocate a new string, copy it and add null termination.
-        llvm::Value *p = lfortran_str_copy(str, idx1, idx2);
+        llvm::Value *step = llvm::ConstantInt::get(context, llvm::APInt(32, 1));
+        llvm::Value *present = llvm::ConstantInt::get(context, llvm::APInt(1, 1));
+        llvm::Value *p = lfortran_str_slice(str, idx1, idx2, step, present, present);
 
         tmp = builder->CreateAlloca(character_type, nullptr);
         builder->CreateStore(p, tmp);
@@ -4945,7 +4947,7 @@ class ASRToLLVMVisitor : public ASR::BaseVisitor<ASRToLLVMVisitor>
         llvm::Value *idx = tmp;
         this->visit_expr_wrapper(x.m_arg, true);
         llvm::Value *str = tmp;
-        tmp = lfortran_str_copy(str, idx, idx);
+        tmp = lfortran_str_item(str, idx);
     }
 
     void visit_StringSection(const ASR::StringSection_t& x) {
diff --git a/src/libasr/runtime/lfortran_intrinsics.c b/src/libasr/runtime/lfortran_intrinsics.c
@@ -849,27 +849,20 @@ LFORTRAN_API char* _lfortran_strrepeat_c(char* s, int32_t n)
     return dest_char;
 }
 
-// idx1 and idx2 both start from 1
-LFORTRAN_API char* _lfortran_str_copy(char* s, int32_t idx1, int32_t idx2) {
+// idx starts from 1
+LFORTRAN_API char* _lfortran_str_item(char* s, int32_t idx) {
 
     int s_len = strlen(s);
-    if(idx1 > s_len || idx1 <= (-1*s_len)){
-        printf("String index out of Bounds\n");
+    int original_idx = idx - 1;
+    if (idx < 1) idx += s_len;
+    if (idx < 1 || idx >= s_len + 1) {
+        printf("String index: %d is out of Bounds\n", original_idx);
         exit(1);
     }
-    if(idx1 <= 0) {
-        idx1 = s_len + idx1;
-    }
-    if(idx2 <= 0) {
-        idx2 = s_len + idx2;
-    }
-    char* dest_char = (char*)malloc(idx2-idx1+2);
-    for (int i=idx1; i <= idx2; i++)
-    {
-        dest_char[i-idx1] = s[i-1];
-    }
-    dest_char[idx2-idx1+1] = '\0';
-    return dest_char;
+    char* res = (char*)malloc(2);
+    res[0] = s[idx-1];
+    res[1] = '\0';
+    return res;
 }
 
 LFORTRAN_API char* _lfortran_str_slice(char* s, int32_t idx1, int32_t idx2, int32_t step,
diff --git a/src/libasr/runtime/lfortran_intrinsics.h b/src/libasr/runtime/lfortran_intrinsics.h
@@ -168,6 +168,7 @@ LFORTRAN_API int8_t* _lfortran_realloc(int8_t* ptr, int32_t size);
 LFORTRAN_API int8_t* _lfortran_calloc(int32_t count, int32_t size);
 LFORTRAN_API void _lfortran_free(char* ptr);
 LFORTRAN_API void _lfortran_string_init(int size_plus_one, char *s);
+LFORTRAN_API char* _lfortran_str_item(char* s, int32_t idx);
 LFORTRAN_API char* _lfortran_str_slice(char* s, int32_t idx1, int32_t idx2, int32_t step,
                         bool idx1_present, bool idx2_present);
 LFORTRAN_API int32_t _lfortran_iand32(int32_t x, int32_t y);
diff --git a/src/lpython/semantics/python_ast_to_asr.cpp b/src/lpython/semantics/python_ast_to_asr.cpp
@@ -3135,19 +3135,6 @@ class CommonVisitor : public AST::BaseVisitor<Struct> {
             }
             ai.m_right = index;
             if (ASRUtils::is_character(*type)) {
-                ASR::expr_t* val = ASRUtils::expr_value(index);
-                if (val && ASR::is_a<ASR::IntegerConstant_t>(*val)) {
-                    if (ASR::down_cast<ASR::IntegerConstant_t>(val)->m_n < 0) {
-                        // Replace `x[-1]` to `x[len(x)+(-1)]`
-                        ASR::ttype_t *int_type = ASRUtils::TYPE(ASR::make_Integer_t(
-                                                        al, loc, 4, nullptr, 0));
-                        ASR::expr_t *list_len = ASRUtils::EXPR(ASR::make_StringLen_t(
-                                    al, loc, value, int_type, nullptr));
-                        ASR::expr_t *neg_idx = ASRUtils::expr_value(index);
-                        index = ASRUtils::EXPR(ASR::make_IntegerBinOp_t(al, loc,
-                            list_len, ASR::binopType::Add, neg_idx, int_type, nullptr));
-                    }
-                }
                 index = index_add_one(loc, index);
                 ai.m_right = index;
                 tmp = ASR::make_StringItem_t(al, loc, value, index, type, nullptr);
diff --git a/tests/reference/runtime-test_str_01-50bdf2f.json b/tests/reference/runtime-test_str_01-50bdf2f.json
@@ -0,0 +1,13 @@
+{
+    "basename": "runtime-test_str_01-50bdf2f",
+    "cmd": "lpython {infile}",
+    "infile": "tests/runtime_errors/test_str_01.py",
+    "infile_hash": "5f1245b9162a004ea90aee813606b7a14a2d06802c8eb2ebd6220f9b",
+    "outfile": null,
+    "outfile_hash": null,
+    "stdout": "runtime-test_str_01-50bdf2f.stdout",
+    "stdout_hash": "ae5584858d62f3df08abdd365ea09fed0a7aa75f0f10698a7f7c2508",
+    "stderr": null,
+    "stderr_hash": null,
+    "returncode": 1
+}
diff --git a/tests/reference/runtime-test_str_01-50bdf2f.stdout b/tests/reference/runtime-test_str_01-50bdf2f.stdout
@@ -0,0 +1 @@
+String index: -4 is out of Bounds
diff --git a/tests/reference/runtime-test_str_02-c38ba27.json b/tests/reference/runtime-test_str_02-c38ba27.json
@@ -0,0 +1,13 @@
+{
+    "basename": "runtime-test_str_02-c38ba27",
+    "cmd": "lpython {infile}",
+    "infile": "tests/runtime_errors/test_str_02.py",
+    "infile_hash": "63853d644565a26ea82a41221d8fe11d9fc20d9132a36a76e6aba5e6",
+    "outfile": null,
+    "outfile_hash": null,
+    "stdout": "runtime-test_str_02-c38ba27.stdout",
+    "stdout_hash": "59f7b180db70f25c853c552c014ae09c1ee0671dfa0cbe205815c6b9",
+    "stderr": null,
+    "stderr_hash": null,
+    "returncode": 1
+}
diff --git a/tests/reference/runtime-test_str_02-c38ba27.stdout b/tests/reference/runtime-test_str_02-c38ba27.stdout
@@ -0,0 +1 @@
+String index: -8 is out of Bounds
diff --git a/tests/runtime_errors/test_str_01.py b/tests/runtime_errors/test_str_01.py
@@ -0,0 +1,11 @@
+from ltypes import i32
+
+
+def test_issue_1536():
+    x: str
+    x = "ABC"
+    l: i32
+    l = -4
+    print(x[l])
+
+test_issue_1536()
diff --git a/tests/runtime_errors/test_str_02.py b/tests/runtime_errors/test_str_02.py
@@ -0,0 +1,6 @@
+def test_issue_1536():
+    x: str
+    x = "ABC"
+    print(x[-8])
+
+test_issue_1536()
diff --git a/tests/tests.toml b/tests/tests.toml
@@ -991,6 +991,14 @@ run = true
 filename = "runtime_errors/test_list_02.py"
 run = true
 
+[[test]]
+filename = "runtime_errors/test_str_01.py"
+run = true
+
+[[test]]
+filename = "runtime_errors/test_str_02.py"
+run = true
+
 [[test]]
 filename = "runtime_errors/test_assert_01.py"
 run_with_dbg = true

Original file line number	Diff line number	Diff line change
`@@ -1211,8 +1211,7 @@ R"(`
`1211`	`1211`	`std::string idx = std::move(src);`
`1212`	`1212`	`this->visit_expr(*x.m_arg);`
`1213`	`1213`	`std::string str = std::move(src);`
`1214`		`- src = "(char )memcpy((char )calloc(2U, sizeof(char)), "`
`1215`		`- + str + " + " + idx + " - 1, 1U)";`
	`1214`	`+ src = "_lfortran_str_item(" + str + ", " + idx + ")";`
`1216`	`1215`	`}`
`1217`	`1216`
`1218`	`1217`	`void visit_StringLen(const ASR::StringLen_t &x) {`