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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
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
----------------
aprantl wrote:
> Should this be code 19?
Yes, fixed.

================
Comment at: docs/LangRef.rst:629
@@ -627,3 +628,3 @@
                          [, section "name"] [, comdat [($name)]]
-                         [, align <Alignment>]
+                         [, align <Alignment>] (, !name !N)*
 
----------------
aprantl wrote:
> Should this be 
> ```
> (!<name> !<N>)*
> ```
> ?
Maybe, but we do have other sigil-variable forms in these grammars, and I think the intent is clear enough.

(If we did want to change this, there's another one like this for functions.)

================
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) {
----------------
aprantl wrote:
> I think we need to guard against corrupted input here.
> 
> ```
>  if (Record.size() % 2)
>      return error("Invalid record");
> ```
> 
Added assert (see below) and guard to the `GLOBALVAR_ATTACHMENT` case above.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4202
@@ -4174,3 +4201,3 @@
         return error("Invalid record");
       if (RecordLength % 2 == 0) {
         // A function attachment.
----------------
aprantl wrote:
> 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.
Added assert above.

I think we need to be able to accept an odd number of fields here for instruction metadata attachments (and presumably would need to keep accepting them for upgrades even if we change the representation).

================
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 {
----------------
aprantl wrote:
> 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.
I started using variables for all tests in this file.


http://reviews.llvm.org/D20074