[PATCH] D25313: Add asan hooks to Recycler and ArrayRecycler
Justin Bogner via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 6 15:12:14 PDT 2016
Nicolai Hähnle <nhaehnle at gmail.com> writes:
> nhaehnle created this revision.
> nhaehnle added reviewers: chandlerc, bogner.
> nhaehnle added a subscriber: llvm-commits.
>
> Already fix some use-after-frees.
Please split the MergeConsecutiveStores change into its own review - I
think it's okay but it will be easier to review without the other
changes mixed in. The other use-after-free fixes all LGTM and can just
be committed.
I'll reply to your llvm-dev post regarding how to go about getting the
hooks themselves in.
>
> https://reviews.llvm.org/D25313
>
> Files:
> include/llvm/CodeGen/SelectionDAGNodes.h
> include/llvm/Support/ArrayRecycler.h
> include/llvm/Support/Recycler.h
> lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>
> Index: lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> +++ lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> @@ -446,10 +446,11 @@
> /// This is a helper function for MergeConsecutiveStores. When the source
> /// elements of the consecutive stores are all constants or all extracted
> /// vector elements, try to merge them into one larger store.
> - /// \return True if a merged store was created.
> - bool MergeStoresOfConstantsOrVecElts(SmallVectorImpl<MemOpLink> &StoreNodes,
> - EVT MemVT, unsigned NumStores,
> - bool IsConstantSrc, bool UseVector);
> + /// \return number of stores that were merged into a merged store (always
> + /// a prefix of \p StoreNode).
> + unsigned MergeStoresOfConstantsOrVecElts(
> + SmallVectorImpl<MemOpLink> &StoreNodes, EVT MemVT, unsigned NumStores,
> + bool IsConstantSrc, bool UseVector);
>
> /// This is a helper function for MergeConsecutiveStores.
> /// Stores that may be merged are placed in StoreNodes.
> @@ -466,8 +467,10 @@
>
> /// Merge consecutive store operations into a wide store.
> /// This optimization uses wide integers or vectors when possible.
> - /// \return True if some memory operations were changed.
> - bool MergeConsecutiveStores(StoreSDNode *N);
> + /// \return number of stores that were merged into a merged store (the
> + /// affected nodes are stored as a prefix in \p StoreNodes).
> + unsigned MergeConsecutiveStores(StoreSDNode *N,
> + SmallVectorImpl<MemOpLink> &StoreNodes);
>
> /// \brief Try to transform a truncation where C is a constant:
> /// (trunc (and X, C)) -> (and (trunc X), (trunc C))
> @@ -2198,8 +2201,8 @@
> SDValue Op1 = Node->getOperand(1);
> SDValue combined;
> for (SDNode::use_iterator UI = Op0.getNode()->use_begin(),
> - UE = Op0.getNode()->use_end(); UI != UE; ++UI) {
> - SDNode *User = *UI;
> + UE = Op0.getNode()->use_end(); UI != UE;) {
> + SDNode *User = *UI++;
> if (User == Node || User->use_empty())
> continue;
> // Convert the other matching node(s), too;
> @@ -11288,12 +11291,12 @@
> return DAG.getBuildVector(Ty, SL, BuildVector);
> }
>
> -bool DAGCombiner::MergeStoresOfConstantsOrVecElts(
> +unsigned DAGCombiner::MergeStoresOfConstantsOrVecElts(
> SmallVectorImpl<MemOpLink> &StoreNodes, EVT MemVT,
> unsigned NumStores, bool IsConstantSrc, bool UseVector) {
> // Make sure we have something to merge.
> if (NumStores < 2)
> - return false;
> + return 0;
>
> int64_t ElementSizeBytes = MemVT.getSizeInBits() / 8;
> LSBaseSDNode *FirstInChain = StoreNodes[0].MemNode;
> @@ -11413,7 +11416,7 @@
> }
> }
>
> - return true;
> + return NumStores;
> }
>
> void DAGCombiner::getStoreMergeAndAliasCandidates(
> @@ -11555,21 +11558,22 @@
> return true;
> }
>
> -bool DAGCombiner::MergeConsecutiveStores(StoreSDNode* St) {
> +unsigned DAGCombiner::MergeConsecutiveStores(
> + StoreSDNode* St, SmallVectorImpl<MemOpLink> &StoreNodes) {
> if (OptLevel == CodeGenOpt::None)
> - return false;
> + return 0;
>
> EVT MemVT = St->getMemoryVT();
> int64_t ElementSizeBytes = MemVT.getSizeInBits() / 8;
> bool NoVectors = DAG.getMachineFunction().getFunction()->hasFnAttribute(
> Attribute::NoImplicitFloat);
>
> // This function cannot currently deal with non-byte-sized memory sizes.
> if (ElementSizeBytes * 8 != MemVT.getSizeInBits())
> - return false;
> + return 0;
>
> if (!MemVT.isSimple())
> - return false;
> + return 0;
>
> // Perform an early exit check. Do not bother looking at stored values that
> // are not constants, loads, or extracted vector elements.
> @@ -11581,38 +11585,36 @@
> StoredVal.getOpcode() == ISD::EXTRACT_SUBVECTOR);
>
> if (!IsConstantSrc && !IsLoadSrc && !IsExtractVecSrc)
> - return false;
> + return 0;
>
> // Don't merge vectors into wider vectors if the source data comes from loads.
> // TODO: This restriction can be lifted by using logic similar to the
> // ExtractVecSrc case.
> if (MemVT.isVector() && IsLoadSrc)
> - return false;
> + return 0;
>
> // Only look at ends of store sequences.
> SDValue Chain = SDValue(St, 0);
> if (Chain->hasOneUse() && Chain->use_begin()->getOpcode() == ISD::STORE)
> - return false;
> + return 0;
>
> // Save the LoadSDNodes that we find in the chain.
> // We need to make sure that these nodes do not interfere with
> // any of the store nodes.
> SmallVector<LSBaseSDNode*, 8> AliasLoadNodes;
>
> // Save the StoreSDNodes that we find in the chain.
> - SmallVector<MemOpLink, 8> StoreNodes;
> -
> getStoreMergeAndAliasCandidates(St, StoreNodes, AliasLoadNodes);
>
> // Check if there is anything to merge.
> if (StoreNodes.size() < 2)
> - return false;
> + return 0;
>
> // only do dependence check in AA case
> bool UseAA = CombinerAA.getNumOccurrences() > 0 ? CombinerAA
> : DAG.getSubtarget().useAA();
> if (UseAA && !checkMergeStoreCandidatesForDependencies(StoreNodes))
> - return false;
> + return 0;
>
> // Sort the memory operands according to their distance from the
> // base pointer. As a secondary criteria: make sure stores coming
> @@ -11740,7 +11742,7 @@
> // consecutive loads).
> if (StoreValOpcode != ISD::EXTRACT_VECTOR_ELT &&
> StoreValOpcode != ISD::EXTRACT_SUBVECTOR)
> - return false;
> + return 0;
>
> // Find a legal type for the vector store.
> unsigned Elts = i + 1;
> @@ -11807,14 +11809,14 @@
> }
>
> if (LoadNodes.size() < 2)
> - return false;
> + return 0;
>
> // If we have load/store pair instructions and we only have two values,
> // don't bother.
> unsigned RequiredAlignment;
> if (LoadNodes.size() == 2 && TLI.hasPairedLoad(MemVT, RequiredAlignment) &&
> St->getAlignment() >= RequiredAlignment)
> - return false;
> + return 0;
>
> LoadSDNode *FirstLoad = cast<LoadSDNode>(LoadNodes[0].MemNode);
> unsigned FirstLoadAS = FirstLoad->getAddressSpace();
> @@ -11888,7 +11890,7 @@
> NumElem = std::min(LastLegalType, NumElem);
>
> if (NumElem < 2)
> - return false;
> + return 0;
>
> // Collect the chains from all merged stores.
> SmallVector<SDValue, 8> MergeStoreChains;
> @@ -11960,7 +11962,7 @@
> }
> }
>
> - return true;
> + return NumElem;
> }
>
> SDValue DAGCombiner::replaceStoreChain(StoreSDNode *ST, SDValue BetterChain) {
> @@ -12205,14 +12207,27 @@
> if (!LegalTypes) {
> bool EverChanged = false;
>
> - do {
> + for (;;) {
> // There can be multiple store sequences on the same chain.
> // Keep trying to merge store sequences until we are unable to do so
> // or until we merge the last store on the chain.
> - bool Changed = MergeConsecutiveStores(ST);
> - EverChanged |= Changed;
> - if (!Changed) break;
> - } while (ST->getOpcode() != ISD::DELETED_NODE);
> + SmallVector<MemOpLink, 8> StoreNodes;
> + unsigned NumChanged = MergeConsecutiveStores(ST, StoreNodes);
> + if (!NumChanged)
> + break;
> +
> + EverChanged = true;
> +
> + bool ChangedST = false;
> + for (unsigned i = 0; i < NumChanged; ++i) {
> + if (StoreNodes[i].MemNode == ST) {
> + ChangedST = true;
> + break;
> + }
> + }
> + if (ChangedST)
> + break;
> + }
>
> if (EverChanged)
> return SDValue(N, 0);
> Index: include/llvm/Support/Recycler.h
> ===================================================================
> --- include/llvm/Support/Recycler.h
> +++ include/llvm/Support/Recycler.h
> @@ -43,13 +43,17 @@
>
> FreeNode *pop_val() {
> auto *Val = FreeList;
> + // Always unpoison the full Size, because the combiner relies on being able
> + // to morph SDNodes into MachineSDNodes.
> + __asan_unpoison_memory_region(FreeList, Size);
> FreeList = FreeList->Next;
> return Val;
> }
>
> void push(FreeNode *N) {
> N->Next = FreeList;
> FreeList = N;
> + __asan_poison_memory_region(FreeList, Size);
> }
>
> public:
> Index: include/llvm/Support/ArrayRecycler.h
> ===================================================================
> --- include/llvm/Support/ArrayRecycler.h
> +++ include/llvm/Support/ArrayRecycler.h
> @@ -48,6 +48,8 @@
> FreeList *Entry = Bucket[Idx];
> if (!Entry)
> return nullptr;
> + __asan_unpoison_memory_region(Entry,
> + sizeof(T) * Capacity::get(Idx).getSize());
> Bucket[Idx] = Entry->Next;
> return reinterpret_cast<T*>(Entry);
> }
> @@ -60,6 +62,8 @@
> Bucket.resize(size_t(Idx) + 1);
> Entry->Next = Bucket[Idx];
> Bucket[Idx] = Entry;
> + __asan_poison_memory_region(Entry,
> + sizeof(T) * Capacity::get(Idx).getSize());
> }
>
> public:
> Index: include/llvm/CodeGen/SelectionDAGNodes.h
> ===================================================================
> --- include/llvm/CodeGen/SelectionDAGNodes.h
> +++ include/llvm/CodeGen/SelectionDAGNodes.h
> @@ -923,7 +923,10 @@
>
> inline SDValue::SDValue(SDNode *node, unsigned resno)
> : Node(node), ResNo(resno) {
> - assert((!Node || ResNo < Node->getNumValues()) &&
> + // Explicitly check for !ResNo to avoid use-after-free, because there are
> + // callers that use SDValue(N, 0) with a deleted N to indicate successful
> + // combines.
> + assert((!Node || !ResNo || ResNo < Node->getNumValues()) &&
> "Invalid result number for the given node!");
> assert(ResNo < -2U && "Cannot use result numbers reserved for DenseMaps.");
> }
More information about the llvm-commits
mailing list