[Lldb-commits] [clang] [lldb] [clang][RecordLayoutBuilder] Be stricter about inferring packed-ness in ExternalLayouts (PR #97443)
https://github.com/efriedma-quic commented:
If I'm understanding correctly, the way this currently works is that you do
normal field layout, then if you discover that the actual offset of a field is
less than the offset normal field layout would produce, you assume the struct
is packed. This misses cases where a struct is packed, but the packing doesn't
affect the offset of any of the fields. But as you note, this can't be fixed
without adjusting the overall architecture.
There's an issue with the current implementation: it skips fields which
actually are packed, I think. Consider the following:
```
struct Empty {};
struct __attribute((packed)) S {
[[no_unique_address]] Empty a,b,c,d;
char x;
int y;
};
S s;
```
In this case, the field "y" is both overlapping, and at a packed offset.
Really, you don't want to check for overlap; you want to ignore empty fields.
(Non-empty fields can't overlap.)
https://github.com/llvm/llvm-project/pull/97443
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: It's not that hard to compute "no-data": non-RecordDecls are never no-data, RecordDecls are no-data if they don't have a vtable pointer (isDynamicClass()), and all fields are no-data. We can save it in the CGRecordLayout. Assuming that's the route we want to go, of course, as opposed to just making LLDB add no_unique_address markings to fields. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote:
> > couldn't the inverse be true, then - that codegen should ignore if
> > something isZeroSize or not?
>
> Just to clarify, is the suggestion here to remove the special handling of
> `isZeroSize` in the RecordLayoutBuilder?
We currently need to distinguish between empty fields and non-empty fields:
various parts of CodeGen expect to be able to get the LLVM struct field
associated with a non-empty clang field. Maybe we can reduce that dependency,
but that would be a deeper refactoring.
But we don't really care whether an empty field is formally "zero-size", so we
could instead just check if the field is empty.
The change would be a bit wider than just RecordLayoutBuilder; there are other
places in CodeGen that check isZeroSize for similar reasons.
> > That would mean if someone wrote `struct Empty {}; struct Z { Empty a,b,c;
> > }`, we'd lower it to `{ [3 x i8] }` instead of `{%Empty, %Empty, %Empty}`,
> > which is a bit ugly. Other than that, sure, I guess we could do that.
>
> Ah, fair enough. Glad to understand and I don't feel /super/ strongly either
> way. Though it might help with confidence if codegen didn't depend on this
> property at all (that it depends on the property a bit may make it harder to
> detect if later codegen depends on the property in a real/ABI-breaking way).
I think we have enough regression tests and assertions to detect breakage from
minor adjustments here.
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote:
That would mean if someone wrote `struct Empty {}; struct Z { Empty a,b,c; }`,
we'd lower it to `{ [3 x i8] }` instead of `{%Empty, %Empty, %Empty}`, which is
a bit ugly. Other than that, sure, I guess we could do that.
https://github.com/llvm/llvm-project/pull/93809
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: Oh, in this particular case, the issue isn't the computed datasize, it's that FieldDecl::isZeroSize() returns the wrong result. If that's the case, maybe we can just change FieldDecl::isZeroSize() to say the field is zero size? So essentially, we pretend all empty fields are no_unique_address. Nothing in codegen should care if we treat an non-zero-size empty field as if it's zero-size. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: > > The correct answer here is probably to fix the sizes in the RecordLayout > > itself; in particular, the DataSize of the members. > > That would be ideal, but also means we'd have to reflect the various C++ > attributes that affect layout in DWARF. Avoiding adding such > language-specific constructs to DWARF is what partly motivated this patch. Given the offsets and sizes of the members of a struct, you can compute the datasize as the offset plus the size of the last member. That isn't really correct for POD structs, but the CGRecordLayout won't care: it can't tell the difference between padding that's illegal to reuse, vs. padding that the frontend chose not to reuse for some other reason. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)
efriedma-quic wrote: I'm skeptical it's correct to skip all the assertions like this; the assertions are there to ensure the layout of the LLVM IR type matches the layout provided by the RecordLayout. If the LLVM IR layout is wrong, address-related computations will be wrong, and ultimately you'll miscompile. We don't really care whether the RecordLayout is consistent with language rules. The correct answer here is probably to fix the sizes in the RecordLayout itself; in particular, the DataSize of the members. https://github.com/llvm/llvm-project/pull/93809 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [llvm] [libc] [mlir] [flang] [lldb] [clang] [libcxx] [mlir] Skip invalid test on big endian platform (s390x) (PR #80246)
@@ -0,0 +1,27 @@
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+// Decoding the attribute does not work on big-endian platforms currently
+// XFAIL: target=s390x-{{.*}}
efriedma-quic wrote:
LLVM tests use "host-byteorder-little-endian"; maybe we can do the same thing
here? (You might need to modify the MLIR lit.cfg.py.)
Not sure how much this matters, given the decline of big-endian PPC, but it
would make the intent clearer.
https://github.com/llvm/llvm-project/pull/80246
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] f8499d5 - Emit the correct flags for the PROC CodeView Debug Symbol
Author: Daniel Paoliello
Date: 2023-05-16T10:58:10-07:00
New Revision: f8499d5709e37b4e9a6d2a39c385cfd2c00bad6e
URL:
https://github.com/llvm/llvm-project/commit/f8499d5709e37b4e9a6d2a39c385cfd2c00bad6e
DIFF:
https://github.com/llvm/llvm-project/commit/f8499d5709e37b4e9a6d2a39c385cfd2c00bad6e.diff
LOG: Emit the correct flags for the PROC CodeView Debug Symbol
The S_LPROC32_ID and S_GPROC32_ID CodeView Debug Symbols have a flags
field which LLVM has had the values for (in the ProcSymFlags enum) but
has never actually set.
These flags are used by Microsoft-internal tooling that leverages debug
information to do binary analysis.
Modified LLVM to set the correct flags:
- ProcSymFlags::HasOptimizedDebugInfo - always set, as this indicates that
debug info is present for optimized builds (if debug info is not emitted
for optimized builds, then LLVM won't emit a debug symbol at all).
- ProcSymFlags::IsNoReturn and ProcSymFlags::IsNoInline - set if the
function has the NoReturn or NoInline attributes respectively.
- ProcSymFlags::HasFP - set if the function requires a frame pointer (per
TargetFrameLowering::hasFP).
Per discussion in review, XFAIL'ing lldb test until someone working on
lldb has a chance to look at it.
Differential Revision: https://reviews.llvm.org/D148761
Added:
Modified:
lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
llvm/test/DebugInfo/COFF/fpo-realign-alloca.ll
llvm/test/DebugInfo/COFF/fpo-realign-vframe.ll
llvm/test/DebugInfo/COFF/frameproc-flags.ll
llvm/test/DebugInfo/COFF/function-options.ll
llvm/test/DebugInfo/COFF/inlining-header.ll
llvm/test/DebugInfo/COFF/inlining.ll
llvm/test/DebugInfo/COFF/long-name.ll
llvm/test/DebugInfo/COFF/multifunction.ll
llvm/test/DebugInfo/COFF/simple.ll
llvm/test/DebugInfo/COFF/types-array.ll
llvm/test/DebugInfo/COFF/types-basic.ll
llvm/test/MC/AArch64/coff-debug.ll
Removed:
diff --git a/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
b/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
index 9057d01c25840..1cb20a4036382 100644
--- a/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
+++ b/lldb/test/Shell/SymbolFile/PDB/function-nested-block.test
@@ -2,6 +2,7 @@ REQUIRES: system-windows, lld
RUN: %build --compiler=clang-cl --nodefaultlib --output=%t.exe
%S/Inputs/FunctionNestedBlockTest.cpp
RUN: lldb-test symbols -find=function -file FunctionNestedBlockTest.cpp -line
4 %t.exe | FileCheck --check-prefix=CHECK-FUNCTION %s
RUN: lldb-test symbols -find=block -file FunctionNestedBlockTest.cpp -line 4
%t.exe | FileCheck --check-prefix=CHECK-BLOCK %s
+XFAIL: *
CHECK-FUNCTION: Found 1 functions:
CHECK-FUNCTION: name = "{{.*}}", mangled = "{{_?}}main"
diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
index ce5fe6139f918..8161de57b58e0 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
@@ -1160,7 +1160,14 @@ void CodeViewDebug::emitDebugInfoForFunction(const
Function *GV,
OS.AddComment("Function section index");
OS.emitCOFFSectionIndex(Fn);
OS.AddComment("Flags");
-OS.emitInt8(0);
+ProcSymFlags ProcFlags = ProcSymFlags::HasOptimizedDebugInfo;
+if (FI.HasFramePointer)
+ ProcFlags |= ProcSymFlags::HasFP;
+if (GV->hasFnAttribute(Attribute::NoReturn))
+ ProcFlags |= ProcSymFlags::IsNoReturn;
+if (GV->hasFnAttribute(Attribute::NoInline))
+ ProcFlags |= ProcSymFlags::IsNoInline;
+OS.emitInt8(static_cast(ProcFlags));
// Emit the function display name as a null-terminated string.
OS.AddComment("Function name");
// Truncate the name so we won't overflow the record length field.
@@ -1480,6 +1487,7 @@ void CodeViewDebug::beginFunctionImpl(const
MachineFunction *MF) {
CurFn->EncodedLocalFramePtrReg = EncodedFramePtrReg::StackPtr;
CurFn->EncodedParamFramePtrReg = EncodedFramePtrReg::StackPtr;
} else {
+ CurFn->HasFramePointer = true;
// If there is an FP, parameters are always relative to it.
CurFn->EncodedParamFramePtrReg = EncodedFramePtrReg::FramePtr;
if (CurFn->HasStackRealignment) {
diff --git a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
index 495822a6e6532..29445b31e7e74 100644
--- a/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
+++ b/llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.h
@@ -191,6 +191,8 @@ class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public
DebugHandlerBase {
bool HasStackRealignment = false;
bool HaveLineInfo = false;
+
+bool HasFramePointer = false;
};
FunctionInfo *CurFn = nullptr;
diff --git a/llvm/test/DebugInfo/COFF/fpo-realign-alloca.ll
b/llvm/test/DebugInf
