GaloisInc / llvm-pretty

An llvm pretty printer inspired by the haskell llvm binding
Other
28 stars 15 forks source link

Question about the nullability of diieName #29

Closed langston-barrett closed 5 years ago

langston-barrett commented 5 years ago

The definition of DIImportedEntity contains a non-nullable String field, diieName.

I'm reading a bitcode file that has a lot of IMPORTED_ENTITY declarations with a 5th field of value 0, e.g.:

    <IMPORTED_ENTITY op0=0 op1=8 op2=6150 op3=15730 op4=73 op5=0 op6=15715/>
    <IMPORTED_ENTITY op0=0 op1=8 op2=6150 op3=15734 op4=75 op5=0 op6=15715/>
    <IMPORTED_ENTITY op0=0 op1=8 op2=6150 op3=15738 op4=76 op5=0 op6=15715/>

In the official C++ parser, this name is obtained with the function getMDString. However, in other locations where the official parser uses getMDString, you provide a Maybe String, for instance in the parsing of DIGlobalVariable (c++, hs).

So, two questions:

  1. When reading the C++ source, how can one tell whether the string should be a Maybe?
  2. Is the current definition of DIImportedEntity correct, or should diieName be a Maybe String?
elliottt commented 5 years ago

To answer your questions:

  1. The source is really hard to read. LLVM tries to maintain compatibility for some number of versions back, and that yields a parser with some cruft. The way that I decided if something should be nullable was usually based on the textual format; if strings were nullable in the rendered metadata, then I would make it nullable in llvm-pretty.
  2. There's a good chance that the definition should just change to String now. The format of the metadata has changed a lot over the years, so this being optional could be to satisfy an older format.
langston-barrett commented 5 years ago

For (2), I was thinking just the reverse, actually: that diieName is currently a String but perhaps it should be a Maybe String. I'll take a look at the textual output and see if there are any empty strings in that field.

langston-barrett commented 5 years ago

I think there is another issue at the root of this problem.