Use the Largest Type for Globals

[LRFLEW]

I had encountered a weird issue where references to stdout weren't being matched by reccmp in my project that's using /ML (i.e. statically linking the runtime). Looking into it, MSVC defines stdout is a macro relative to the global variable _iob. Looking at the output of cvdump, I noticed that there are two entries for this variable in the GLOBALS section, one right after the other, but with different type IDs.

S_GDATA32: [0003:00000050], Type:             0x10C7, _iob
S_GDATA32: [0003:00000050], Type:             0x10BF, _iob

Looking at the TYPES section, we can see the types are defined as such:

0x10c7 : Length = 14, Leaf = 0x1503 LF_ARRAY
	Element type = 0x10A3
	Index type = T_SHORT(0011)
	length = 640
	Name = 

0x10bf : Length = 14, Leaf = 0x1503 LF_ARRAY
	Element type = 0x10A3
	Index type = T_SHORT(0011)
	length = 0
	Name = 

I believe what's happening here is that the first global is based on the variable's definition in libc.lib, whereas the second global is based on the variable's declaration in stdio.h, which looks like this:

_CRTIMP extern FILE _iob[];

Since the declaration is unsized, it seems that the debug information for the obj files that only see the declaration defaulted to a length of zero. This causes it to differ from the obj file where it's actually defined, and when linking all the obj files together, the pdb ends up keeping both types for it, hence the two global entries.

Currently, when reccmp encounters a second global at the same location, it overrides the old size with the new size. Since the declaration's type comes after the definition's one, this results in the global being recorded as having a size of zero. While this particular case could be fixed by simply keeping the first type instead, I chose to make it more robust, and have it keep whichever type is larger. That way, when reccmp encounters multiple globals at the same location, it will always keep whichever size covers the most cases, regardless of the order they appear in.

[jonschz]

Same as the other PRs - I'd be great if you could add a test. I think this one shouldn't be too difficult - it may be sufficient run CvdumpAnalysis and insert a mock CvdumpParser with pre-set fields, most of them empty arrays.

[LRFLEW]

I had started looking into adding a test after I made the PR (wanted to make it first in case there was any discussion needed with it), and I think testing it could be even easier than that. Instead of mocking CvdumpParser, it could just create an actual parser instance and pass in a few test section examples, similar to what test_cvdump_parser.py does in #207, then pass the result to CvdumpAnalysis and check the results. It would be a bit more than a "unit" test, as it would be testing how the two classes integrate together, but should be simple enough to make. I just need the time to make what is essentially an entire mock cvdump.

[jonschz]

An integration test is also fine. What I'm interested in is any reasonable test that fails on the previous code and passes on the new code.

[disinvite]

What other situations could cause two GLOBALS entries for an address? Is this array example the only case so far? Would it be enough to just ignore the entry where size=0?

[LRFLEW]

I did some quick experimentation with my project (which isn't very big right now, so it wasn't a comprehensive test), and couldn't find any examples of multiple global entries where the other entries weren't just zero-length arrays. They didn't always appear in the same order, and there were a few that appeared more than twice, but it does seem like exactly one entry was non-zero length. I was worried that there might be cases where there wasn't a non-zero length entry (namely with the initialization stuff like the __xc_a and __xc_z variables), but I was mistaken.

My main reason for doing it this way was because I figured it was more resilient. For example, if there was some zero-sized global, removing all zero-sized entries would end up losing that symbol. I don't think that's legal in C or C++, but I thought it might be possible with assembly. However, I'm not seeing any examples of that in libc (for the version of MSVC I'm using), so that's probably not a concern.

The only other case I figured it could happen is, if having a different but compatible declaration in a header can cause this issue, then a declaration in a header was a different incompatible type could as well. For example, if a variable is defined in a class file as int, but declared in a header as extern short, that could also cause this issue. Granted, if someone did that, they probably have bigger issues, and without knowing which was the declaration and which was the definition, it may not be a good idea to just take the larger of the two.

Looking at this again, would it be better to simply ignore zero-length arrays, and throw a warning / error if it sees two incompatible globals entries (eg. different names or different non-zero sizes)?

[disinvite]

Looking at this again, would it be better to simply ignore zero-length arrays, and throw a warning / error if it sees two incompatible globals entries (eg. different names or different non-zero sizes)?

I think this is the way to go with the information we currently have.

In the specific example of the zero length array, it would be better to use the type key with a non-zero size.

If there are two competing types that each have a non-zero size, log a warning so we can review the cvdump output later. Using the largest size (the way this PR is written now) seems like a reasonable choice to me. In the absence of real-world data around this situation, I think we could make the case for any option.

[disinvite]

The new tests added by #237 fail if we don't have type information. That's because we no longer establish the DATA node here and we hit the upcoming except block before it gets created.

We should always have type information from the PDB, but it's not currently a requirement to create a global variable entity.

[disinvite]

@LRFLEW what do you think of this? It avoids the issue of whether a type's size can have int | None or should default to zero, our current CI error from mypy. I would say the former is always the case, but let's keep the changes contained to this class.

for glo in parser.globals:
    key = NodeKey(glo.section, glo.offset)
    if key not in node_dict:
        node_dict[key] = CvdumpNode.from_node_key(key)
    
    node_dict[key].node_type = EntityType.DATA
    if node_dict[key].friendly_name is None:
        node_dict[key].friendly_name = glo.name

    try:
        # Check our types database for type information.
        # If we did not parse the TYPES section, we can only
        # get information for built-in "T_" types.
        g_info = parser.types.get(glo.type)

        # mypy coercion.
        current_size = node_dict[key].confirmed_size or 0
        new_size = g_info.size or 0

        if new_size >= current_size:
            # Replace variable name with the GLOBALS
            # entry that has the largest type.
            node_dict[key].friendly_name = glo.name
            node_dict[key].confirmed_size = g_info.size
            node_dict[key].data_type = g_info

    except (CvdumpKeyError, CvdumpIntegrityError):
        # We can still create the variable entity without type information.
        pass