[clang][CGRecordLayout] Remove dependency on isZeroSize (#96422)

This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801

The

[clang][CGRecordLayout] Remove dependency on isZeroSize (#96422)

This is a follow-up from the conversation starting at
https://github.com/llvm/llvm-project/pull/93809#issuecomment-2173729801

The root problem that motivated the change are external AST sources that
compute `ASTRecordLayout`s themselves instead of letting Clang compute
them from the AST. One such example is LLDB using DWARF to get the
definitive offsets and sizes of C++ structures. Such layouts should be
considered correct (modulo buggy DWARF), but various assertions and
lowering logic around the `CGRecordLayoutBuilder` relies on the AST
having `[[no_unique_address]]` attached to them. This is a
layout-altering attribute which is not encoded in DWARF. This causes us
LLDB to trip over the various LLVM<->Clang layout consistency checks.
There has been precedent for avoiding such layout-altering attributes
from affecting lowering with externally-provided layouts (e.g., packed
structs).

This patch proposes to replace the `isZeroSize` checks in
`CGRecordLayoutBuilder` (which roughly means "empty field with
[[no_unique_address]]") with checks for
`CodeGen::isEmptyField`/`CodeGen::isEmptyRecord`.

**Details**
The main strategy here was to change the `isZeroSize` check in
`CGRecordLowering::accumulateFields` and
`CGRecordLowering::accumulateBases` to use the `isEmptyXXX` APIs
instead, preventing empty fields from being added to the `Members` and
`Bases` structures. The rest of the changes fall out from here, to
prevent lookups into these structures (for field numbers or base
indices) from failing.

Added `isEmptyRecordForLayout` and `isEmptyFieldForLayout` (open to
better naming suggestions). The main difference to the existing
`isEmptyRecord`/`isEmptyField` APIs, is that the `isEmptyXXXForLayout`
counterparts don't have special treatment for `unnamed bitfields`/arrays
and also treat fields of empty types as if they had
`[[no_unique_address]]` (i.e., just like the `AsIfNoUniqueAddr` in
`isEmptyField` does).

show more ...

[clang] Remove rdar links; NFC

We have a new policy in place making links to private resources
something we try to avoid in source and test files. Normally, we'd
organically switch to the new policy

[clang] Remove rdar links; NFC

We have a new policy in place making links to private resources
something we try to avoid in source and test files. Normally, we'd
organically switch to the new policy rather than make a sweeping change
across a project. However, Clang is in a somewhat special circumstance
currently: recently, I've had several new contributors run into rdar
links around test code which their patch was changing the behavior of.
This turns out to be a surprisingly bad experience, especially for
newer folks, for a handful of reasons: not understanding what the link
is and feeling intimidated by it, wondering whether their changes are
actually breaking something important to a downstream in some way,
having to hunt down strangers not involved with the patch to impose on
them for help, accidental pressure from asking for potentially private
IP to be made public, etc. Because folks run into these links entirely
by chance (through fixing bugs or working on new features), there's not
really a set of problematic links to focus on -- all of the links have
basically the same potential for causing these problems. As a result,
this is an omnibus patch to remove all such links.

This was not a mechanical change; it was done by manually searching for
rdar, radar, radr, and other variants to find all the various
problematic links. From there, I tried to retain or reword the
surrounding comments so that we would lose as little context as
possible. However, because most links were just a plain link with no
supporting context, the majority of the changes are simple removals.

Differential Review: https://reviews.llvm.org/D158071

show more ...

Revert "Remove rdar links; NFC"

This reverts commit d618f1c3b12effd0c2bdb7d02108d3551f389d3d.
This commit wasn't reviewed ahead of time and significant concerns were
raised immediately after it land

Revert "Remove rdar links; NFC"

This reverts commit d618f1c3b12effd0c2bdb7d02108d3551f389d3d.
This commit wasn't reviewed ahead of time and significant concerns were
raised immediately after it landed. According to our developer policy
this warrants immediate revert of the commit.

https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

Differential Revision: https://reviews.llvm.org/D155509

show more ...

Remove rdar links; NFC

This removes links to rdar, which is an internal bug tracker that the
community doesn't have visibility into.

See further discussion at:
https://discourse.llvm.org/t/code-rev

Remove rdar links; NFC

This removes links to rdar, which is an internal bug tracker that the
community doesn't have visibility into.

See further discussion at:
https://discourse.llvm.org/t/code-review-reminder-about-links-in-code-commit-messages/71847

show more ...

[Clang] Convert some tests to opaque pointers (NFC)

These tests all require some adjustments to make sure that struct
types still get generated, mostly done by stripping pointer
indirections.

Some

[Clang] Convert some tests to opaque pointers (NFC)

These tests all require some adjustments to make sure that struct
types still get generated, mostly done by stripping pointer
indirections.

Some of this may no longer test the situation it was originally
intended for, e.g. the issue from pr18962 just doesn't really
exist anymore with opaque pointers, as we no longer generate
recursive types.

show more ...

[OpaquePtrs][Clang] Add -no-opaque-pointers to tests (NFC)

This adds -no-opaque-pointers to clang tests whose output will
change when opaque pointers are enabled by default. This is
intended to be p

[OpaquePtrs][Clang] Add -no-opaque-pointers to tests (NFC)

This adds -no-opaque-pointers to clang tests whose output will
change when opaque pointers are enabled by default. This is
intended to be part of the migration approach described in
https://discourse.llvm.org/t/enabling-opaque-pointers-by-default/61322/9.

The patch has been produced by replacing %clang_cc1 with
%clang_cc1 -no-opaque-pointers for tests that fail with opaque
pointers enabled. Worth noting that this doesn't cover all tests,
there's a remaining ~40 tests not using %clang_cc1 that will need
a followup change.

Differential Revision: https://reviews.llvm.org/D123115

show more ...

CodeGen: Don't crash when initializing pointer-to-member fields in bases

Clang uses two types to talk about a C++ class, the
NonVirtualBaseLLVMType and the LLVMType. Previously, we would allow one

CodeGen: Don't crash when initializing pointer-to-member fields in bases

Clang uses two types to talk about a C++ class, the
NonVirtualBaseLLVMType and the LLVMType. Previously, we would allow one
of these to be packed and the other not.

This is problematic. If both don't agree on a common subset of fields,
then routines like getLLVMFieldNo will point to the wrong field. Solve
this by copying the 'packed'-ness of the complete type to the
non-virtual subobject. For this to work, we need to take into account
the non-virtual subobject's size and alignment when we are computing the
layout of the complete object.

This fixes PR21089.

llvm-svn: 218577

show more ...

Fix DOS-style newlines.

A previous patch r210330 (and possibly another) introduced DOS-style newlines
into a UNIX newline formatted file.

Patch by Mark Heffernan (http://reviews.llvm.org/D4046)

l

Fix DOS-style newlines.

A previous patch r210330 (and possibly another) introduced DOS-style newlines
into a UNIX newline formatted file.

Patch by Mark Heffernan (http://reviews.llvm.org/D4046)

llvm-svn: 210369

show more ...

Fixed Assert In CGRecordLowering
Prior to this patch, CGRecordLower assumed that virtual bases could not
be placed before the nvsize of an object. This isn't true in Itanium
mode, virtual bases ar

Fixed Assert In CGRecordLowering
Prior to this patch, CGRecordLower assumed that virtual bases could not
be placed before the nvsize of an object. This isn't true in Itanium
mode, virtual bases are placed at dsize rather than vnsize and in the
case of zero sized non-virtual bases nvsize can be larger than dsize.
This patch fixes CGRecordLowering to avoid an assert and to clip
bitfields properly in this case. A test case is included.

llvm-svn: 207280

show more ...

Complete Rewrite of CGRecordLayoutBuilder

CGRecordLayoutBuilder was aging, complex, multi-pass, and shows signs of
existing before ASTRecordLayoutBuilder. It redundantly performed many
layout ope

Complete Rewrite of CGRecordLayoutBuilder

CGRecordLayoutBuilder was aging, complex, multi-pass, and shows signs of
existing before ASTRecordLayoutBuilder. It redundantly performed many
layout operations that are now performed by ASTRecordLayoutBuilder and
asserted that the results were the same. With the addition of support
for the MS-ABI, such as placement of vbptrs, vtordisps, different
bitfield layout and a variety of other features, CGRecordLayoutBuilder
was growing unwieldy in its redundancy.

This patch re-architects CGRecordLayoutBuilder to not perform any
redundant layout but rather, as directly as possible, lower an
ASTRecordLayout to an llvm::type. The new architecture is significantly
smaller and simpler than the CGRecordLayoutBuilder and contains fewer
ABI-specific code paths. It's also one pass.

The architecture of the new system is described in the comments. For the
most part, the new system simply takes all of the fields and bases from
an ASTRecordLayout, sorts them, inserts padding and dumps a record.
Bitfields, unions and primary virtual bases make this process a bit more
complicated. See the inline comments.

In addition, this patch updates a few lit tests due to the fact that the
new system computes more accurate llvm types than CGRecordLayoutBuilder.
Each change is commented individually in the review.

Differential Revision: http://llvm-reviews.chandlerc.com/D2795

llvm-svn: 201907

show more ...

Add a missing check in CodeGen of packed classes with vtables. <rdar://problem/11324125>.

llvm-svn: 155689

Record layout requires not just a definition, but a complete
definition. Assert this. Change IR generation to not try to
aggressively emit the IR translation of a record during its
own definition.

Record layout requires not just a definition, but a complete
definition. Assert this. Change IR generation to not try to
aggressively emit the IR translation of a record during its
own definition. Fixes PR10912.

llvm-svn: 141350

show more ...

clang side to match the LLVM IR type system rewrite patch.

llvm-svn: 134831

Use the right type name.

llvm-svn: 129674

When laying out bases in, always try the "base subobject" LLVM type. If it
turns out that a field or base needs to be laid out in the tail padding of
the base, CGRecordLayoutBuilder::ResizeLastBaseFi

When laying out bases in, always try the "base subobject" LLVM type. If it
turns out that a field or base needs to be laid out in the tail padding of
the base, CGRecordLayoutBuilder::ResizeLastBaseFieldIfNecessary will convert
it to an array of i8.

I've audited the new test results to make sure that they are still valid. I've
also verified that we pass a self-host with this change.

This (finally) fixes PR5589!

llvm-svn: 129673

show more ...

Put each test in class-layout.cpp into a separate namespace.

llvm-svn: 120925

Use a more appropriate LLVM type for the vtable pointer.

llvm-svn: 103078

Baby steps towards fixing PR5589. If a class needs a vtable pointer, add one.

llvm-svn: 91545

Update tests to use %clang_cc1 instead of 'clang-cc' or 'clang -cc1'.
- This is designed to make it obvious that %clang_cc1 is a "test variable"
which is substituted. It is '%clang_cc1' instead o

Update tests to use %clang_cc1 instead of 'clang-cc' or 'clang -cc1'.
- This is designed to make it obvious that %clang_cc1 is a "test variable"
which is substituted. It is '%clang_cc1' instead of '%clang -cc1' because it
can be useful to redefine what gets run as 'clang -cc1' (for example, to set
a default target).

llvm-svn: 91446

show more ...

No need to add tail padding if the resulting LLVM struct type will have the same size as the final record size.

llvm-svn: 90820

Eliminate &&s in tests.
- 'for i in $(find . -type f); do sed -e 's#\(RUN:.*[^ ]\) *&& *$#\1#g' $i | FileUpdate $i; done', for the curious.

llvm-svn: 86430

Use the CGRecordLayoutBuilder even if there are no fields, because in C++ an empty class will have a padding byte.

llvm-svn: 77205