[PATCH] D20435: [codeview] Adding support for CodeView types

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Hi all,
Thanks a lot for all your valuable comments.

Reid, I agree with you that we can reuse TypeRecord instead of introducing a new CodeViewType.
I also understand your design and approach. And I am willing to refactor my patch to suit them.

However, I must mention two concerns:

1. Can we be sure that we can generate the correct index for each type record during creation of that record?
  - In some cases you want to revisit the type record you generated to fix some of its attributes (like the attribute "ClassOptions::ContainsNestedClass").
2. PDB support
  - I know that you do not want to support PDB, but we must keep it into consideration.
  - When emitting types to PDB we do not know the index of each type record before we actually emit the record.
  - In the current design we create the type record index when we write the record (before we emit it to the object/pdb file).

Once again, the approach you have in mind can work if we solve the above two issues.
One way to solve them is to allow fixing data in the written record, although it would be better if we can delay the creation of the record buffer till we know the final value of the data.

Any thoughts?


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.h:108
@@ +107,3 @@
+  // All types in the order they should be emitted.
+  typedef std::vector<CodeViewType *> CodeViewTypeTable;
+  CodeViewTypeTable TypeTable;
----------------
dblaikie wrote:
> Could you use a MapVector to keep the vector and map together. Or, alternatively, to save space, you could create a temporary vector at the point of emission, and sort based on the type id.
It will not work once I add support of class types.
Because the map will have two dimensions, [Class-Type][DIType] -> CodeViewType.
And the order we want to save in the vector will get lost in this two dimentson map.

================
Comment at: test/DebugInfo/COFF/function-type.ll:6-8
@@ +5,5 @@
+; 'clang -O0 -c -g -gcodeview -S -emit-llvm' on the following code:
+;  1: int foo(char c, short s, long l, float f) {
+;  2: return 0;
+;  3: }
+
----------------
dblaikie wrote:
> There's a function type you could test in the basic-types test case, perhaps? (not a hard requirement - it's a tricky tradeoff between multiple mini tests in a single test case, versus pulling things out into entirely separate test cases to make it simpler/more obvious)
You are right, but I wanted to have a better test for  LF_ARGLIST, the function in the basic-types test is very simple.
Also, having a test for each type rather than having one test for all types will help understanding the reason for the failure faster.
Finally, we do not have a lot of types, something around 10, is that too much for tests?

Notice that I am not trying to have a test for each CodeView type entry, and will merge some entries in same test where it make sense.


http://reviews.llvm.org/D20435