impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Impala Public Jenkins (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4929: Safe concurrent access to IR function call graph
Date Fri, 10 Mar 2017 02:00:48 GMT
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4929: Safe concurrent access to IR function call graph
......................................................................


IMPALA-4929: Safe concurrent access to IR function call graph

Previously, LlvmCodeGen creates a map (fn_refs_map_) which
maps an IR function to its set of callees. That map was initialized
once at Impalad's start time and it is supposed to be read-only
afterwards.

However, the map was unintentionally modified by its user
LlvmCodeGen::MaterializeFunctionHelper() due to the use of
operator []. In particular, that operator may create a new
entry in the map if it doesn't exist already. This is possible
because the map initially only contains entries for functions
with at least one callee function. Using the operator [] with
functions with no callee function may modify the map. Since the
map is shared by all fragment instances, such unsafe concurrent
modification can cause missing or wrong callee functions to be
materialized, leading to failure to resolve symbols in LLVM.

This change fixes the problem above by:
1. Create an entry for all functions even if it has no callee.
   In fact, LlvmCodeGen::LinkModule() assumes all functions
   defined in main module will have entries in the map.

2. Introduce a new class CodegenCallGraph which encapsulates
   the map described above. The map was established once at
   initialization time and there is no interface to modify the
   map afterwards, thus preventing any accidental modification
   to the map at run time.

Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
Reviewed-on: http://gerrit.cloudera.org:8080/6326
Reviewed-by: Michael Ho <kwho@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/CMakeLists.txt
A be/src/codegen/codegen-callgraph.cc
A be/src/codegen/codegen-callgraph.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
5 files changed, 188 insertions(+), 81 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Michael Ho: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/6326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I1acd6bad80341121c8189d817e0fe62f2862f28a
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>

Mime
View raw message