On 12/14/20 10:48 PM, Nikhil Benesch wrote: > On 12/14/20 10:34 PM, Ian Lance Taylor wrote: >> On Mon, Dec 14, 2020 at 7:14 PM Nikhil Benesch wrote: >>> Also godump now emits a dummy `type _u?pad128_t struct {}` entry, >>> so we just suppress that and conditionally add it back. >> >> I don't understand this bit. Why are we seeing an empty struct >> definition, and why is this change the right fix? >> >> Is the problem that because we don't know how to emit the definition >> of the struct, godump.c winds up emitting an empty definition? That >> doesn't sound like the right thing to do for this case. > > That's a good question. u?pad128_t is clearly getting marked as a > potential dummy type. But then you'd think it'd *also* get marked as > an invalid type, and then find_dummy_types would suppress it. I misunderstood how find_dummy_types worked. Here is a correct description. find_dummy_types will output a dummy type for an entry in pot_dummy_types if: (1) we never observed a definition for that type name, or (2) we observed an invalid definition for that type name. I don't actually understand why condition (2) is considered. Since types that refer to invalid types are themselves marked as invalid, godump will recursively comment out all types that refer even transitively to an invalid dummy type. So it doesn't seem necessary to emit a dummy type in condition (2). Ian, you added this behavior way back in 2012: https://github.com/gcc-mirror/gcc/commit/3eb9e389a6460b6bd9400a3f4acf88fb2e258e07 Perhaps you remember why? If consensus is this behavior is wrong, I've attached a patch (invalid-dummy.patch) that removes condition (2) from find_dummy_types along with some new test cases. I don't have enough context to say whether this patch is the right solution though. ---- Separately, I'm actually not sure why we're extracting _u?pad128_t from gen-sysinfo.go at all. Nothing in libgo refers to those types. And no types in [runtime_]sysinfo.go could refer to them either, because if they did they'd *also* be marked as invalid (right?). So I suspect we can just nuke those extractions from mk[r]sysinfo.sh. I've attached a patch that does so (solaris-godump.patch). I'm still waiting on my compile to complete, but initial signs look promising. If this patch looks good, I'll submit it upstream tomorrow. The nice thing about this approach, if it works, is that it works both with and without the change to find_dummy_types, so we can consider that patch independently. Cheers, Nikhil