[PATCH] D33892: Align definition of DW_OP_plus with DWARF spec [1/3]

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:549
+      // Up-to-date!
+      NewExpr.append(Expr.begin(), Expr.end());
+      return Error::success();
----------------
You'll notice that for up-to-date expressions it does an unnecessary copy to NewExpr.
I did this to make the interface to upgradeDIExpression() more clear, i.e. the new expression is always in NewExpr and the old one is left alone. Otherwise the caller would need to figure out where the result is, either in the first or the second argument.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:553
+
+    if (FromVersion <= 2) {
+      // Change DW_OP_plus to DW_OP_plus_uconst.
----------------
I'm starting with upgrading from version 2 to 0 (rather than in reverse order), so that we only need to do the 'copy' to NewExpr once.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:536
 
+  void upgradeMetadataExpression(uint64_t FromVersion,
+                                 MutableArrayRef<uint64_t> Expr,
----------------
aprantl wrote:
> aprantl wrote:
> > Let's rename this to `upgradeDIExpression`.
> Also, `FromVersion` is unused.
This will be used now that I've moved the whole switch statement to upgradeDIExpression().


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:569
+      // Continue with remaining elements
+      SubExpr = SubExpr.slice(HistoricSize);
+    }
----------------
aprantl wrote:
> Copying the expression here seems to be unnecessary, but I assume the reason is that the DW_OP_minus upgrade cannot be done in place.
correct.


================
Comment at: test/Bitcode/DIExpression-deref.ll:15
+; CHECK: !DIExpression(DW_OP_plus_uconst, 0, DW_OP_deref, DW_OP_LLVM_fragment, 8, 32)
+!6 = !DIExpression(DW_OP_deref, DW_OP_plus_uconst, 0, DW_OP_LLVM_fragment, 8, 32)
+; CHECK: !DIExpression(DW_OP_plus_uconst, 0, DW_OP_deref)
----------------
aprantl wrote:
> This seems wrong. This file is supposed to be the disassembled version of the .bc file, so it should contain the old DIExpression to test the bitcode upgrade. (The CHECK is fine though).
Well spotted! Thanks.
I did the same for DIGlobalVariableExpression.ll (that has a corresponding .bc file too).


https://reviews.llvm.org/D33892