[PATCH] D20074: Add support for metadata attachments for global variables.

[Adrian Prantl via llvm-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Some more minor comments, but otherwise this looks good from my end. Thanks!


================
Comment at: docs/BitCodeFormat.rst:851
@@ +850,3 @@
+
+The ``GLOBALVAR_ATTACHMENT`` record (code 18) describes the metadata
+attachments for a global variable. The ``valueid`` is the value index for
----------------
Should this be code 19?

================
Comment at: docs/LangRef.rst:629
@@ -627,3 +628,3 @@
                          [, section "name"] [, comdat [($name)]]
-                         [, align <Alignment>]
+                         [, align <Alignment>] (, !name !N)*
 
----------------
Should this be 
```
(!<name> !<N>)*
```
?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4160
@@ +4159,3 @@
+std::error_code BitcodeReader::parseGlobalObjectAttachment(
+    GlobalObject &GO, ArrayRef<uint64_t> Record) {
+  for (unsigned I = 0, E = Record.size(); I != E; I += 2) {
----------------
I think we need to guard against corrupted input here.

```
 if (Record.size() % 2)
     return error("Invalid record");
```


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4202
@@ -4174,3 +4201,3 @@
         return error("Invalid record");
       if (RecordLength % 2 == 0) {
         // A function attachment.
----------------
Oh. Given that this is here we can do an assert above. I still wonder whether an odd number of fields shouldn't be a hard error here.

================
Comment at: test/Assembler/metadata.ll:8
@@ -4,3 +7,3 @@
 ; CHECK-LABEL: @test
-; CHECK: ret void, !bar !4, !foo !3
+; CHECK: ret void, !foo !5, !bar !6
 define void @test() !dbg !1 {
----------------
Should we use variables here or renumber the metadata in the testcase here? It's odd to check for something different than the input here.


http://reviews.llvm.org/D20074