[PATCH] D95329: [llvm-link] Fix crash when materializing appending global

[Artem Belevich via Phabricator via llvm-commits]

tra added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ValueMapper.cpp:822-824
+      // mapAppendingVariable call can change AppendingInits if initalizer for
+      // the variable depends on another appending global, because of that inits
+      // need to be extracted and updated before the call.
----------------
sdmitriev wrote:
> tra wrote:
> > Does the crash happen because `mapAppendingVariable` attempts to iterate over an array that may change by the loop?
> > If that's the case, the fix (making a temp copy with relevant items) should probably be done there.
> > 
> For the IR from test `mapAppendingVariable` call causes a new item to be added to the `AppendingInits` vector, but this item gets incorrectly cleaned up from it by the `resize` call which follows `mapAppendingVariable`. As a result we are getting `PrefixSize` == -1 while processing the second appending global which leads to a crash.
I see. I'd rename `Inits` -> `NewInits` or `UnmappedInits` to make it more clear that we only want to process new entries.
Perhaps `PrefixSize` could also use a more descriptive name, too. `NumMappedInits` ?


================
Comment at: llvm/lib/Transforms/Utils/ValueMapper.cpp:825
+      // need to be extracted and updated before the call.
+      SmallVector<Constant *, 16> Inits(drop_begin(AppendingInits, PrefixSize));
+      AppendingInits.resize(PrefixSize);
----------------
What's the typical number of inits do you expect to see in practice? 
Presumably, it's smaller than that of `AppendingInits` itself, which is 16. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95329/new/

https://reviews.llvm.org/D95329