You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "anujphadke (Code Review)" <ge...@cloudera.org> on 2018/01/29 23:24:52 UTC
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
anujphadke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9154
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
IMPALA-6008: Creating a UDF from a shared library with a .ll extenion
crashes impala
Impala crashes on creating a UDF from a shared library (.so file) which
was renamed to have .ll extension. CreateFile() call in GetSymbols() fails
and returns on error and does not close the codegen object. This patch
closes the codegen object on failure. This avoids hitting a DCHECK later
up in the stack.
The chain of failures also invokes the DiagnosticHandlerFn. RuntimeState
object is NULL when the DiagnosticHandlerFn gets called in this case.
This change also adds a check before accessing it for logging.
Testing:
Created a UDF using a shared object with a .ll extension (renamed a .so
object). This change returns the following error instead of crashing -
[localhost:21000] > create function foo4 (string, string) returns string
location '/tmp/test.ll' symbol='MyAwesomeUdf';
Query: create function foo4 (string, string) returns string location
'/tmp/test.ll' symbol='MyAwesomeUdf'
ERROR: AnalysisException: Could not load binary: /tmp/test.ll
Could not parse module /tmp/test.15137.0.ll: llvm.bitcode:2
Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
---
M be/src/codegen/llvm-codegen.cc
1 file changed, 9 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/9154/1
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 1
Gerrit-Owner: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
Hello Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9154
to look at the new patch set (#3).
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
IMPALA-6008: Creating a UDF from a shared library with a .ll extenion
crashes impala
Impala crashes on creating a UDF from a shared library (.so file) which
was renamed to have .ll extension. CreateFile() call in GetSymbols()
fails and returns on error and does not close the codegen object. This
patch closes the codegen object on failure. This avoids hitting a DCHECK
later up in the stack.
The chain of failures also invokes the DiagnosticHandlerFn. RuntimeState
object is NULL when the DiagnosticHandlerFn gets called in this case.
This change also adds a check before accessing it for logging.
[localhost:21000] > create function foo4 (string, string) returns string
location '/tmp/bad_udf.ll' symbol='MyAwesomeUdf';
Query: create function foo4 (string, string) returns string location
'/tmp/bad_udf.ll' symbol='MyAwesomeUdf'
ERROR: AnalysisException: Could not load binary: /tmp/bad_udf.ll
LLVM diagnostic error: Invalid bitcode signature
Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
---
M be/src/codegen/llvm-codegen.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
M tests/query_test/test_udfs.py
3 files changed, 27 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/9154/3
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 3
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
IMPALA-6008: Creating a UDF from a shared library with a .ll extenion
crashes impala
Impala crashes on creating a UDF from a shared library (.so file) which
was renamed to have .ll extension. CreateFile() call in GetSymbols()
fails and returns on error and does not close the codegen object. This
patch closes the codegen object on failure. This avoids hitting a DCHECK
later up in the stack.
The chain of failures also invokes the DiagnosticHandlerFn. RuntimeState
object is NULL when the DiagnosticHandlerFn gets called in this case.
This change also adds a check before accessing it for logging.
[localhost:21000] > create function foo4 (string, string) returns string
location '/tmp/bad_udf.ll' symbol='MyAwesomeUdf';
Query: create function foo4 (string, string) returns string location
'/tmp/bad_udf.ll' symbol='MyAwesomeUdf'
ERROR: AnalysisException: Could not load binary: /tmp/bad_udf.ll
LLVM diagnostic error: Invalid bitcode signature
Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Reviewed-on: http://gerrit.cloudera.org:8080/9154
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/llvm-codegen.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
M tests/query_test/test_udfs.py
3 files changed, 48 insertions(+), 14 deletions(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 8
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9154/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9154/2//COMMIT_MSG@12
PS2, Line 12: failsand
nit: fails and
http://gerrit.cloudera.org:8080/#/c/9154/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9154/2/be/src/codegen/llvm-codegen.cc@280
PS2, Line 280: ss << "Could not parse module " << module_name << ": " << tmp_module.getError();
since the diagnostic handler is called, it would be worth adding the message here. Please see LinkModuleFromLocalFs() (Line:318 onwards) for an example of how to fetch the diagnostic message from the handler.
http://gerrit.cloudera.org:8080/#/c/9154/2/be/src/codegen/llvm-codegen.cc@1315
PS2, Line 1315: Status status = CreateFromFile(nullptr, &pool, nullptr, file, module_id, &codegen);
: if (!status.ok()) {
: codegen->Close();
: return status;
: }
i hit the same problem earlier when I tried loading a lib generated from an updated version of llvm into a lower version, glad this got fixed here.
Seems like this happens whenever llvm encounters a malformed file. Is there anyway we can add a test for this that doesn't involve adding a file? If not, would it make sense to add one, just for this test?
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 2
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Comment-Date: Tue, 30 Jan 2018 00:12:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
IMPALA-6008: Creating a UDF from a shared library with a .ll extenion
crashes impala
Impala crashes on creating a UDF from a shared library (.so file) which
was renamed to have .ll extension. CreateFile() call in GetSymbols()
failsand returns on error and does not close the codegen object. This
patch closes the codegen object on failure. This avoids hitting a DCHECK
later up in the stack.
The chain of failures also invokes the DiagnosticHandlerFn. RuntimeState
object is NULL when the DiagnosticHandlerFn gets called in this case.
This change also adds a check before accessing it for logging.
Testing:
Created a UDF using a shared object with a .ll extension (renamed a .so
object). This change returns the following error instead of crashing -
[localhost:21000] > create function foo4 (string, string) returns string
location '/tmp/test.ll' symbol='MyAwesomeUdf';
Query: create function foo4 (string, string) returns string location
'/tmp/test.ll' symbol='MyAwesomeUdf'
ERROR: AnalysisException: Could not load binary: /tmp/test.ll
Could not parse module /tmp/test.15137.0.ll: llvm.bitcode:2
Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
---
M be/src/codegen/llvm-codegen.cc
1 file changed, 9 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/9154/2
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 2
Gerrit-Owner: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 6
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 22:43:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9154/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9154/5/be/src/codegen/llvm-codegen.cc@220
PS5, Line 220: return (*codegen)->Init(std::move(loaded_module));
We should still need to close *codegen if this fails. I.e. I think we need something like:
status = (*codegen)->Init(...);
if (!status.ok()) goto error;
return Status::OK();
http://gerrit.cloudera.org:8080/#/c/9154/5/be/src/codegen/llvm-codegen.cc@251
PS5, Line 251: return (*codegen)->Init(std::move(loaded_module));
Same comment applies here.
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 5
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 01:21:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9154
to look at the new patch set (#4).
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
IMPALA-6008: Creating a UDF from a shared library with a .ll extenion
crashes impala
Impala crashes on creating a UDF from a shared library (.so file) which
was renamed to have .ll extension. CreateFile() call in GetSymbols()
fails and returns on error and does not close the codegen object. This
patch closes the codegen object on failure. This avoids hitting a DCHECK
later up in the stack.
The chain of failures also invokes the DiagnosticHandlerFn. RuntimeState
object is NULL when the DiagnosticHandlerFn gets called in this case.
This change also adds a check before accessing it for logging.
[localhost:21000] > create function foo4 (string, string) returns string
location '/tmp/bad_udf.ll' symbol='MyAwesomeUdf';
Query: create function foo4 (string, string) returns string location
'/tmp/bad_udf.ll' symbol='MyAwesomeUdf'
ERROR: AnalysisException: Could not load binary: /tmp/bad_udf.ll
LLVM diagnostic error: Invalid bitcode signature
Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
---
M be/src/codegen/llvm-codegen.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
M tests/query_test/test_udfs.py
3 files changed, 39 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/9154/4
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 4
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 3:
(4 comments)
Thanks for fixing this, it would be good to get this merged. The test coverage looks good and the fix is valid, I just noticed the test may be flaky and the structure of the error handling in the code could be improved to reduce the chance of similar mistakes in future.
http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc@223
PS3, Line 223: CreateFromMemory
This function also has the same bad pattern of not closing 'codegen' on errors - we should probably fix that too while we're here.
http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc@1315
PS3, Line 1315: Status status = CreateFromFile(nullptr, &pool, nullptr, file, module_id, &codegen);
CreateFromFile() should really close the LlvmCodegen object on failure - it's awkward requiring the caller to do it. Might be easier to use the "goto error" pattern with a cleanup block at the end of the CreateFromFile() function.
http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py@332
PS3, Line 332: with open("bad_udf.ll", "w") as f:
The local and HDFS file creation will race if test_udf_errors() runs with itself concurrently. Would be best to use tempfile.mkstemp() for the local file and include the unique_database string in the path of the HDFS file.
http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py@338
PS3, Line 338: check_call
Use call() instead of check_call(), since we want to execute the next cleanup step even on errors.
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 3
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 06:29:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9154
to look at the new patch set (#5).
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
IMPALA-6008: Creating a UDF from a shared library with a .ll extenion
crashes impala
Impala crashes on creating a UDF from a shared library (.so file) which
was renamed to have .ll extension. CreateFile() call in GetSymbols()
fails and returns on error and does not close the codegen object. This
patch closes the codegen object on failure. This avoids hitting a DCHECK
later up in the stack.
The chain of failures also invokes the DiagnosticHandlerFn. RuntimeState
object is NULL when the DiagnosticHandlerFn gets called in this case.
This change also adds a check before accessing it for logging.
[localhost:21000] > create function foo4 (string, string) returns string
location '/tmp/bad_udf.ll' symbol='MyAwesomeUdf';
Query: create function foo4 (string, string) returns string location
'/tmp/bad_udf.ll' symbol='MyAwesomeUdf'
ERROR: AnalysisException: Could not load binary: /tmp/bad_udf.ll
LLVM diagnostic error: Invalid bitcode signature
Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
---
M be/src/codegen/llvm-codegen.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
M tests/query_test/test_udfs.py
3 files changed, 42 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/9154/5
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 5
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 7
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Fri, 23 Feb 2018 03:50:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9154/5/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9154/5/be/src/codegen/llvm-codegen.cc@220
PS5, Line 220: status = (*codegen)->Init(std::move(loaded_module));
> We should still need to close *codegen if this fails. I.e. I think we need
Done
http://gerrit.cloudera.org:8080/#/c/9154/5/be/src/codegen/llvm-codegen.cc@251
PS5, Line 251: module_name, &loaded_module);
> Same comment applies here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 6
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 04:54:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 7
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 22:43:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc@223
PS3, Line 223:
> This function also has the same bad pattern of not closing 'codegen' on err
Done
http://gerrit.cloudera.org:8080/#/c/9154/3/be/src/codegen/llvm-codegen.cc@1315
PS3, Line 1315: // entry:
> CreateFromFile() should really close the LlvmCodegen object on failure - it
Done.
Removed the check in GetSymbols and moved it to CreateFromFile
http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:
http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py@332
PS3, Line 332: # 1. too many arguments
> The local and HDFS file creation will race if test_udf_errors() runs with i
Done
http://gerrit.cloudera.org:8080/#/c/9154/3/tests/query_test/test_udfs.py@338
PS3, Line 338: check_call
> Use call() instead of check_call(), since we want to execute the next clean
Done
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 5
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 21 Feb 2018 08:50:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Bikramjeet Vig,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9154
to look at the new patch set (#6).
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
IMPALA-6008: Creating a UDF from a shared library with a .ll extenion
crashes impala
Impala crashes on creating a UDF from a shared library (.so file) which
was renamed to have .ll extension. CreateFile() call in GetSymbols()
fails and returns on error and does not close the codegen object. This
patch closes the codegen object on failure. This avoids hitting a DCHECK
later up in the stack.
The chain of failures also invokes the DiagnosticHandlerFn. RuntimeState
object is NULL when the DiagnosticHandlerFn gets called in this case.
This change also adds a check before accessing it for logging.
[localhost:21000] > create function foo4 (string, string) returns string
location '/tmp/bad_udf.ll' symbol='MyAwesomeUdf';
Query: create function foo4 (string, string) returns string location
'/tmp/bad_udf.ll' symbol='MyAwesomeUdf'
ERROR: AnalysisException: Could not load binary: /tmp/bad_udf.ll
LLVM diagnostic error: Invalid bitcode signature
Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
---
M be/src/codegen/llvm-codegen.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
M tests/query_test/test_udfs.py
3 files changed, 48 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/54/9154/6
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 6
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9154/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9154/2//COMMIT_MSG@12
PS2, Line 12: fails an
> nit: fails and
Done
http://gerrit.cloudera.org:8080/#/c/9154/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9154/2/be/src/codegen/llvm-codegen.cc@280
PS2, Line 280: return Status(diagnostic_err);
> since the diagnostic handler is called, it would be worth adding the messag
Done
http://gerrit.cloudera.org:8080/#/c/9154/2/be/src/codegen/llvm-codegen.cc@1315
PS2, Line 1315: Status status = CreateFromFile(nullptr, &pool, nullptr, file, module_id, &codegen);
: if (!status.ok()) {
: codegen->Close();
: return status;
: }
> i hit the same problem earlier when I tried loading a lib generated from an
Done
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 3
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Sat, 10 Feb 2018 00:54:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9154 )
Change subject: IMPALA-6008: Creating a UDF from a shared library with a .ll extenion crashes impala
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1989/
--
To view, visit http://gerrit.cloudera.org:8080/9154
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id060668802ca9c80367cdc0e8a823b968d549bbb
Gerrit-Change-Number: 9154
Gerrit-PatchSet: 7
Gerrit-Owner: anujphadke <ap...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Thu, 22 Feb 2018 22:44:09 +0000
Gerrit-HasComments: No