Michael137 wrote: > > > For CGClass, it's not directly tied to the LLVM structure layout, but I'm > > > not sure the generated code would be semantically correct if an "empty" > > > field that isn't isEmpty() overlaps with actual data. > > > > > > I haven't addressed this yet. To clarify, are you referring to > > `addMemcpyableField` and `PushCleanupForField` (both of which check > > `isZeroSize`? So if we generated an overlap between an "empty" (but not > > `isEmpty`/`isZeroSize`) field and a non-empty field, previously > > `addMemcpyableField` wouldn't have considered the field for inclusion in > > the memcpy? So we should adjust the `isZeroSize` here too? Doing so didn't > > reflect in the test-suite, so it's possible there's some missing coverage > > here too > > In addMemcpyableField(), if we conclude a field is not zero-size (and the > other heuristics pass), we generate a memcpy for it. But if the field is > actually empty, it might overlap with some other field, so the memcpy() might > actually end up overwriting something unrelated. Similarly, for > PushCleanupForField, we'll poison any overlapping data. In both cases, if the > field doesn't actually contain any data, we can safely skip the relevant code. > > I wouldn't be surprised if there's missing coverage here.
Makes sense! > It looks like in the latest patch, there's still a couple uses of > isZeroSize() in CGExprConstant? Yea those were the last occurrences I wanted to sanity check since they're calling into `ASTRecordLayout::getFieldOffset` (which is built based on `isZeroSize`) and there's a `NoUniqueAddressAttr` check in there too. But it seemed reasonable to skip empty fields here too. Had to adjust the `CodeGen/2009-06-14-anonymous-union-init.c` test which checks for a `zeroinitializer` (which in the test happened to be for an empty structure, see latest commit). https://github.com/llvm/llvm-project/pull/96422 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
