https://github.com/dwblaikie approved this pull request.
Looks OK - one minor nit, I'd probably avoid defining CLANG_TRAP_PREFIX as a
macro, but instead as an inline or some other form of constant
https://github.com/llvm/llvm-project/pull/79230
___
@@ -27,6 +27,9 @@ namespace llvm {
}
}
+// Prefix of the name of the artificial inline frame.
+#define CLANG_TRAP_PREFIX "__clang_trap_msg"
dwblaikie wrote:
inline StringRef? usual reasons to avoid macros, etc
dwblaikie wrote:
I will say, `-fno-eliminate-unused-debug-types` is a really heavy hammer that
makes debug info much larger - and my understanding was that games tended to
have trouble with large debug builds, so I'd be surprised if this was a great
path forward.
Do you have a small example
dwblaikie wrote:
Yes, the initializers do have to be lazyily evaluated to be a conforming C++
compiler.
eg: this code must compile without error so far as I understand:
```
template
struct t1 {
static constexpr int x = 3 / Value;
};
t1<0> v1;
```
Though it seems MSVC doesn't implement this
dwblaikie wrote:
I think my last comment/question is still open? How/why did the symbol name end
up dropping any llvm/clang component to avoid collisions with other names?
https://github.com/llvm/llvm-project/pull/79230
___
cfe-commits mailing list
dwblaikie wrote:
happy with the test coverage, but I'm probably not the right person to review
th ecode in more detail
https://github.com/llvm/llvm-project/pull/91310
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dwblaikie 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.
Ah, fair enough. Glad to understand and I don't
dwblaikie 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
https://github.com/dwblaikie approved this pull request.
https://github.com/llvm/llvm-project/pull/95174
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dwblaikie wrote:
Congrats on the milestone! Glad to have more of us together on the same version
for all the positive feedback loops, etc :)
https://github.com/llvm/llvm-project/pull/95164
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dwblaikie wrote:
> G'day -- we've got some tests for -O0 that, with this patch applied, generate
> different code with-and-without the `-g` flag, if
> `-fdebug-info-for-profiling` is enabled. Example godbolt:
> https://godbolt.org/z/qooo5Eah1 .
>
> It seems the intention of this patch is
@@ -1830,6 +1830,9 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables
,
if (VTable->hasInitializer())
return;
+ if (RD->shouldEmitInExternalSource())
+return;
dwblaikie wrote:
But this logic (using available externally definitions) is
@@ -1830,6 +1830,9 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables
,
if (VTable->hasInitializer())
return;
+ if (RD->shouldEmitInExternalSource())
+return;
dwblaikie wrote:
Yes, and we do emit them in normal builds above -O0 if the
@@ -3239,6 +3239,12 @@ bool ASTReader::isConsumerInterestedIn(Decl *D) {
if (ES->hasExternalDefinitions(D) == ExternalASTSource::EK_Never)
return true;
+ // The dynamic class defined in a named module is interesting.
+ // The code generator needs to emit its
@@ -1185,6 +1190,21 @@ bool CodeGenVTables::isVTableExternal(const
CXXRecordDecl *RD) {
TSK == TSK_ExplicitInstantiationDefinition)
return false;
+ // Itanium C++ ABI [5.2.3]:
+ // Virtual tables for dynamic classes are emitted as follows:
+ //
+ // - If the
dwblaikie wrote:
Per ( https://llvm.org/docs/GitHub.html#updating-pull-requests ) please don't
force push, makes it hard to follow the changes to the review as they progress.
https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing
@@ -1180,6 +1185,21 @@ bool CodeGenVTables::isVTableExternal(const
CXXRecordDecl *RD) {
TSK == TSK_ExplicitInstantiationDefinition)
return false;
+ // Itanium C++ ABI [5.2.3]:
+ // Virtual tables for dynamic classes are emitted as follows:
+ //
+ // - If the
https://github.com/dwblaikie approved this pull request.
Looks worth a shot. Please commit this at a time where you can respond to bot
fail-mail for a while after the commit.
And ensure the commit message has most/all of the details of the original
commit, plus the original commit and revert
Author: David Blaikie
Date: 2024-05-30T20:36:47Z
New Revision: 6e975ecf5c93c40d2f088425548eb6476332629c
URL:
https://github.com/llvm/llvm-project/commit/6e975ecf5c93c40d2f088425548eb6476332629c
DIFF:
https://github.com/llvm/llvm-project/commit/6e975ecf5c93c40d2f088425548eb6476332629c.diff
@@ -1180,6 +1185,21 @@ bool CodeGenVTables::isVTableExternal(const
CXXRecordDecl *RD) {
TSK == TSK_ExplicitInstantiationDefinition)
return false;
+ // Itanium C++ ABI [5.2.3]:
+ // Virtual tables for dynamic classes are emitted as follows:
+ //
+ // - If the
@@ -1802,6 +1802,12 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables
,
if (VTable->hasInitializer())
return;
+ // If the class is attached to a C++ named module other than the one
+ // we're currently compiling, the vtable should be defined there.
+ if
dwblaikie wrote:
Could probably adjust the assertions to be `assert (debug || whatever)` rather
than `if (!debug) assert(whatever)`.
My understanding/expectation was that these assertions would be removed
entirely - that whatever generated the AST should just be trusted, whether it's
the
dwblaikie wrote:
Thanks! Sorry I didn't get to it sooner
https://github.com/llvm/llvm-project/pull/93767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dwblaikie wrote:
> There is a broken test in CI:
> https://lab.llvm.org/buildbot/#/builders/272/builds/17864
Fixed in ea1ecb50fa831583241fc531153bd2c072955d29
https://github.com/llvm/llvm-project/pull/81545
___
cfe-commits mailing list
Author: David Blaikie
Date: 2024-05-29T23:36:43Z
New Revision: ea1ecb50fa831583241fc531153bd2c072955d29
URL:
https://github.com/llvm/llvm-project/commit/ea1ecb50fa831583241fc531153bd2c072955d29
DIFF:
https://github.com/llvm/llvm-project/commit/ea1ecb50fa831583241fc531153bd2c072955d29.diff
https://github.com/dwblaikie closed
https://github.com/llvm/llvm-project/pull/87018
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/dwblaikie approved this pull request.
Fair enough - let's give it a go.
https://github.com/llvm/llvm-project/pull/81545
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://github.com/dwblaikie commented:
Is there some test coverage that shows that unreferenced variables/functions
aren't included in the output?
https://github.com/llvm/llvm-project/pull/91310
___
cfe-commits mailing list
https://github.com/dwblaikie approved this pull request.
approving mostly on the basis of my own previous approval on phab
https://github.com/llvm/llvm-project/pull/91423
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
@@ -1802,6 +1802,12 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables
,
if (VTable->hasInitializer())
return;
+ // If the class is attached to a C++ named module other than the one
+ // we're currently compiling, the vtable should be defined there.
+ if
@@ -1180,6 +1185,21 @@ bool CodeGenVTables::isVTableExternal(const
CXXRecordDecl *RD) {
TSK == TSK_ExplicitInstantiationDefinition)
return false;
+ // Itanium C++ ABI [5.2.3]:
+ // Virtual tables for dynamic classes are emitted as follows:
+ //
+ // - If the
@@ -1180,6 +1185,21 @@ bool CodeGenVTables::isVTableExternal(const
CXXRecordDecl *RD) {
TSK == TSK_ExplicitInstantiationDefinition)
return false;
+ // Itanium C++ ABI [5.2.3]:
+ // Virtual tables for dynamic classes are emitted as follows:
+ //
+ // - If the
dwblaikie wrote:
(a bunch of compiler-rt tests also use ulimit, but doesn't look like any llvm
core tests do... )
https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dwblaikie wrote:
there's a couple of tests that use `ulimit`
(`clang/test/SemaCXX/PR51712-large-array-constexpr-check-oom.cpp` and
`clang/test/PCH/leakfiles.test`) - so that technique could be used to test this
in a way that's fast enough to check in?
dwblaikie wrote:
Ah, OK - guess this might not be an ABI issue, then - carry on :) (I'll leave
it to other Clang-y folks to do the rest of the review, the ABI issues were my
only concern)
https://github.com/llvm/llvm-project/pull/92103
___
dwblaikie wrote:
> @dwblaikie This patch will bring Clang in line with GCC and MSVC:
> https://godbolt.org/z/nj715zbsW
would the change be ABI incompatible with previous versions of clang? If so,
then it'll need to be versioned in Clang's ClangABI handling, so that platforms
(like Sony's
dwblaikie wrote:
@MaskRay seems like this target might be too niche to go into LLVM at this
time? is it worth considering some bar before accepting such a thing into LLVM,
rather than encouraging folks to maintain such a thing in a branch for now?
@@ -4260,6 +4260,13 @@ defm strict_dwarf : BoolOption<"g", "strict-dwarf",
"the specified version, avoiding features from later versions.">,
NegFlag, BothFlags<[], [ClangOption, CLOption, DXCOption]>>,
Group;
+defm omit_unreferenced_members : BoolOption<"g",
dwblaikie wrote:
> I think the comment about `s/members/methods/` is still outstanding - I agree
> that methods is more descriptive than members.
Yeah, seems I'm outvoted here. I'm a bit of a pedant for the C++ standard
language, which doesn't talk about "methods", only "member functions".
@@ -4260,6 +4260,13 @@ defm strict_dwarf : BoolOption<"g", "strict-dwarf",
"the specified version, avoiding features from later versions.">,
NegFlag, BothFlags<[], [ClangOption, CLOption, DXCOption]>>,
Group;
+defm omit_unreferenced_members : BoolOption<"g",
@@ -2755,7 +2755,7 @@ CGDebugInfo::CreateTypeDefinition(const RecordType *Ty) {
// Collect data fields (including static variables and any initializers).
CollectRecordFields(RD, DefUnit, EltTys, FwdDecl);
- if (CXXDecl)
+ if (CXXDecl &&
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -gomit-unreferenced-members %s
-emit-llvm -o - | FileCheck %s
dwblaikie wrote:
Done
https://github.com/llvm/llvm-project/pull/87018
___
cfe-commits
https://github.com/dwblaikie updated
https://github.com/llvm/llvm-project/pull/87018
>From 6834c245205d1e38a615e97217dada3cd941ed03 Mon Sep 17 00:00:00 2001
From: David Blaikie
Date: Fri, 2 Jun 2023 15:04:14 +
Subject: [PATCH 1/3] [DebugInfo] Add flag to only emit referenced member
dwblaikie wrote:
How's this compare with other implementations clang is trying to be compatible
with (gcc (in the normal clang driver mode) and msvc (in clang-cl mode))? Would
this be an ABI break against them, or is this intended as an ABI fix to align
better with them? Or some third option?
@@ -3424,6 +3445,26 @@ llvm::DIMacroFile
*CGDebugInfo::CreateTempMacroFile(llvm::DIMacroFile *Parent,
return DBuilder.createTempMacroFile(Parent, Line, FName);
}
+llvm::DILocation *CGDebugInfo::CreateTrapFailureMessageFor(
dwblaikie wrote:
Eh, I'm not too
dwblaikie wrote:
Hmm, do other builtins actually end up as symbol names? I think most of them
lower to an instruction or two - I guess this one doesn't lower to an actual
symbol, only a DWARF symbol - but maybe that's still different enough it should
use an llvm or clang in the name...
dwblaikie wrote:
@pogo59 - you might find this interesting in terms of bitrotten tests, etc.
https://github.com/llvm/llvm-project/pull/91854
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dwblaikie wrote:
> Switched to 0x4014 (generic range) to retain linker errors while making
> the experimental status stand out. Add a comment to make the intention
> clearer.
Seems a bit weird/problematic, using something in the reserved range/not in a
user extension space, but I guess
dwblaikie wrote:
> This approahc parallels the strategy used for
> [-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang](https://reviews.llvm.org/D125142),
> albeit a nicer name.
Fair enough - yeah, if we're wordsmithing this. Maybe "allow" would be a good
word, rather
dwblaikie wrote:
Oh, and I take it there's no /official/ extension space in the SHT_* space? The
intent is to standardize it where it lies? (like use 20 for the shipped
version? Or eventually get some lower designation?) I understand tnhe hesitance
to use "SHT_LLVM" or the like (though
dwblaikie wrote:
Presumably this'll be split out into separate reviews for the components here?
(I'd guess llvm-mc then clang integrated assembler, with readobj and yaml2obj
in any order/don't have dependencies, unless they're needed for testing, in
which case I guess they come first?)
I
dwblaikie wrote:
> Though we detect when the types aren't identical and don't try to use them
> interchangeably. The change extends the existing behavior for structs/unions
> to enums.
OK, still a bit confused though - "like in C++", I assume in C++ we reject
mismatched types coming from
@@ -5636,6 +5636,84 @@ void
CGDebugInfo::EmitExternalVariable(llvm::GlobalVariable *Var,
Var->addDebugInfo(GVE);
}
+void CGDebugInfo::EmitPseudoVariable(CGBuilderTy ,
+ llvm::Instruction *Value, QualType Ty) {
+ // Only when -g2 or
@@ -27,6 +27,9 @@ namespace llvm {
}
}
+// Prefix for __builtin_verbose_trap.
+#define CLANG_VERBOSE_TRAP_PREFIX "__llvm_verbose_trap"
dwblaikie wrote:
& not sure it being a compile time constant regex is super important - the
regex compilation probably
@@ -27,6 +27,9 @@ namespace llvm {
}
}
+// Prefix for __builtin_verbose_trap.
+#define CLANG_VERBOSE_TRAP_PREFIX "__llvm_verbose_trap"
dwblaikie wrote:
I think we usually use some llvm or clang prefix in these sort of things, to
reduce the chance of
dwblaikie wrote:
> > Comment in the code should probably mention this as a FIXME and include a
> > reference to the issue?
>
> Sure, added in
> [f78949a](https://github.com/llvm/llvm-project/commit/f78949a07e33017a798c410a102c95455685a9b1)
Thanks!
> > Also, there's another bug here - the
Author: David Blaikie
Date: 2024-04-29T23:50:18Z
New Revision: 1cb33713910501c6352d0eb2a15b7a15e6e18695
URL:
https://github.com/llvm/llvm-project/commit/1cb33713910501c6352d0eb2a15b7a15e6e18695
DIFF:
https://github.com/llvm/llvm-project/commit/1cb33713910501c6352d0eb2a15b7a15e6e18695.diff
dwblaikie wrote:
> > C doesn't have an odr, does it?
>
> For non-C++ "ODR" has a meaning more like "ODR-inspired checks". But there is
> no language rule that would require enforcement and there is no impact on
> linkage (at least during deserialization).
Not sure I'm following the response
dwblaikie wrote:
Comment in the code should probably mention this as a FIXME and include a
reference to the issue?
Also, there's another bug here - the DW_TAG_typedef is in the CU scope, instead
of the struct scope. But if the struct is a non-template, the typedef is in the
struct scope as
dwblaikie wrote:
C doesn't have an odr, does it?
https://github.com/llvm/llvm-project/pull/90298
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -3379,6 +3379,60 @@ Query for this feature with
``__has_builtin(__builtin_debugtrap)``.
Query for this feature with ``__has_builtin(__builtin_trap)``.
+``__builtin_verbose_trap``
dwblaikie wrote:
Ah, OK - that seems like more reason the prefix should be
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fcxx-exceptions -verify %s
+
+#if !__has_builtin(__builtin_verbose_trap)
+#error
+#endif
+
+constexpr char const* constMsg1 = "hello";
+char const* const constMsg2 = "hello";
+char const constMsg3[] = "hello";
+
@@ -3424,6 +3445,26 @@ llvm::DIMacroFile
*CGDebugInfo::CreateTempMacroFile(llvm::DIMacroFile *Parent,
return DBuilder.createTempMacroFile(Parent, Line, FName);
}
+llvm::DILocation *CGDebugInfo::CreateTrapFailureMessageFor(
dwblaikie wrote:
I think I agree
@@ -5636,6 +5636,84 @@ void
CGDebugInfo::EmitExternalVariable(llvm::GlobalVariable *Var,
Var->addDebugInfo(GVE);
}
+void CGDebugInfo::EmitPseudoVariable(CGBuilderTy ,
+ llvm::Instruction *Value, QualType Ty) {
+ // Only when -g2 or
@@ -5636,6 +5636,84 @@ void
CGDebugInfo::EmitExternalVariable(llvm::GlobalVariable *Var,
Var->addDebugInfo(GVE);
}
+void CGDebugInfo::EmitPseudoVariable(CGBuilderTy ,
+ llvm::Instruction *Value, QualType Ty) {
+ // Only when -g2 or
@@ -1787,7 +1787,26 @@ Value *ScalarExprEmitter::VisitMemberExpr(MemberExpr *E)
{
}
}
- return EmitLoadOfLValue(E);
+ llvm::Value *Result = EmitLoadOfLValue(E);
+
+ // If -fdebug_info_for_profiling is specified, emit a pseudo variable and its
dwblaikie wrote:
> Yes it's fixed now.
Please include details of the fix I the patch description
https://github.com/llvm/llvm-project/pull/89036
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dwblaikie wrote:
> The original patch pointed out a regression:
>
> https://github.com/llvm/llvm-project/pull/83124#issuecomment-2060090590
>
> Please contact that person and get a reproducer and make sure you aren't
> breaking them.
Repro was provided further down:
dwblaikie wrote:
Simplified a bit more:
```
template
void f1() {
int (*x)[1] = new int[1][1]; // fails
}
template void f1();
void f2() {
int (*x)[1] = new int[1][1]; // passes
}
```
https://godbolt.org/z/MhaYbvefG - yeah seems pretty clear this is a regression.
Compiles with GCC, fails
dwblaikie wrote:
> Thanks @dwblaikie
>
> > Bit unfortunate to store template parameters in different ways (in the
> > extraData for the alias template, but in the templateParams for the
> > composite types) - but I guess it'd be more invasive to try to represent
> > alias templates as
dwblaikie wrote:
Re: code: Looks about right.
Bit unfortunate to store template parameters in different ways (in the
`extraData` for the alias template, but in the `templateParams` for the
composite types) - but I guess it'd be more invasive to try to represent alias
templates as composite
https://github.com/dwblaikie edited
https://github.com/llvm/llvm-project/pull/85050
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/dwblaikie approved this pull request.
Give it a go
https://github.com/llvm/llvm-project/pull/85050
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -1061,6 +1070,16 @@ CodeGenAction::CreateASTConsumer(CompilerInstance ,
StringRef InFile) {
CI.getPreprocessor().addPPCallbacks(std::move(Callbacks));
}
+ if (CI.getFrontendOpts().GenReducedBMI &&
+ !CI.getFrontendOpts().ModuleOutputPath.empty()) {
+
dwblaikie wrote:
> > I'm a little uncomfortable with adding a new user-facing option for
> > template aliases. Even with that in place, we should not warn and refuse to
> > do what the user asked for, based on DWARF version. -gdwarf-2 -gsplit-dwarf
> > generates a .dwo file claiming to be v2,
dwblaikie wrote:
> As for impact, I believe @namhyung did some measurement for building the
> Linux kernel, and it does not have a significant impact.
That'd surprise me quite a bit - perhaps a self-host build of clang (ideally in
Google's build config, that being the one you and I care about
dwblaikie wrote:
Thanks for continuing to look into this!
@cor3ntin - perhaps you've got some more thoughts on this too?
https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
@@ -1483,10 +1483,15 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl
*D) {
if (D->isThisDeclarationADefinition())
Record.AddCXXDefinitionData(D);
- // Store (what we currently believe to be) the key function to avoid
- // deserializing every method so we can
@@ -41,9 +43,10 @@ Base::~Base() {}
// CHECK: @_ZTSW3Mod4Base = constant
// CHECK: @_ZTIW3Mod4Base = constant
-// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant
-// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant
-// CHECK-INLINE:
@@ -3017,6 +3017,7 @@ defm prebuilt_implicit_modules :
BoolFOption<"prebuilt-implicit-modules",
def fmodule_output_EQ : Joined<["-"], "fmodule-output=">,
Flags<[NoXarchOption]>, Visibility<[ClangOption, CC1Option]>,
+ MarshallingInfoString>,
dwblaikie
@@ -193,6 +193,20 @@ DwarfFissionKind getDebugFissionKind(const Driver ,
const llvm::opt::ArgList ,
llvm::opt::Arg *);
+// Calculate the output path of the module file when compiling a module unit
+//
dwblaikie wrote:
Cleaning up some old branches - @pogo59 @rnk who commented on the original
https://reviews.llvm.org/D152017
I think the only outstanding thing was the flag name, I've renamed it from
`-gincomplete-types` to `-gomit-unreferenced-members` to try to address the
feedback. It's
https://github.com/dwblaikie created
https://github.com/llvm/llvm-project/pull/87018
Complete C++ type information can be quite expensive - and there's
limited value in representing every member function, even those that
can't be called (we don't do similarly for every non-member function
dwblaikie wrote:
> > Reading LLVM IR lit CHECK lines from clang codegen is a bit difficult -
> > could you include some simple examples (perhaps from the new clang tests in
> > this patch) showing the DWARF output just as comments in this review for
> > something more easily glanceable?
>
>
@@ -60,11 +63,11 @@ int use() {
// CHECK-NOT: @_ZTSW3Mod4Base = constant
// CHECK-NOT: @_ZTIW3Mod4Base = constant
-// CHECK: @_ZTVW3Mod4Base = external unnamed_addr
+// CHECK: @_ZTVW3Mod4Base = external
-// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr
@@ -41,9 +43,10 @@ Base::~Base() {}
// CHECK: @_ZTSW3Mod4Base = constant
// CHECK: @_ZTIW3Mod4Base = constant
-// CHECK-INLINE: @_ZTVW3Mod4Base = linkonce_odr {{.*}}unnamed_addr constant
-// CHECK-INLINE: @_ZTSW3Mod4Base = linkonce_odr {{.*}}constant
-// CHECK-INLINE:
@@ -1483,10 +1483,15 @@ void ASTDeclWriter::VisitCXXRecordDecl(CXXRecordDecl
*D) {
if (D->isThisDeclarationADefinition())
Record.AddCXXDefinitionData(D);
- // Store (what we currently believe to be) the key function to avoid
- // deserializing every method so we can
dwblaikie wrote:
> > > > > @iains @dwblaikie Understood. And I thought the major problem is that
> > > > > there are a lot flags from clang modules. And it is indeed confusing.
> > > > > But given we have to introduce new flags in this case, probably we
> > > > > can only try to make it more
dwblaikie wrote:
Yep, the original code still crashes with this PR applied, and the reduced test
case comes down to something identical to the code shown in
https://github.com/llvm/llvm-project/pull/86401#issuecomment-2024151742 with a
stack trace that looks the same as the one caused by the
dwblaikie wrote:
> @dwblaikie Feel free to checkout this patch locally and see if it resolves
> the original issue - I won't merge it until you confirm it works or discover
> another issue that goes beyond the scope of this patch. (e.g. another
> aforementioned issue)
First glance it seems
dwblaikie wrote:
> For those files in the repository that do need CRLF endings, I've adopted a
> policy of eol=crlf which means that git will store them in history with LF,
> but regardless of user config, they'll be checked out in tree with CRLF.
This ^ seems a bit problematic to me, though
dwblaikie wrote:
Hmm, actually - does this fix address /other/ ways a pack could appear, like
this? https://godbolt.org/z/oez8TbGqM
Presumably a pack could appear in a variety of expressions, not just wrapped in
parens - could be in a function call (as in the above example), or nested
https://github.com/dwblaikie approved this pull request.
LGTM, seems consistent with the previous patch - thanks!
https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dwblaikie wrote:
+1 to @iains's comments about being careful about the introduction and naming
of driver flags & probably avoid it in this case, if possible, or try to make
it clearly experimental.
https://github.com/llvm/llvm-project/pull/85050
___
dwblaikie wrote:
Reading LLVM IR lit CHECK lines from clang codegen is a bit difficult - could
you include some simple examples (perhaps from the new clang tests in this
patch) showing the DWARF output just as comments in this review for something
more easily glanceable?
As for flags - I
dwblaikie wrote:
FWIW, @jyknight's example fails to compile with GCC - succeeds with clang 18
release but assert-fails with clang 18 +Asserts build. (not sure about the
original/full internal reproduction - we have a way to run compile with
assertions enaled, but I'm not sure I'm holding it
Author: David Blaikie
Date: 2024-03-13T16:03:16Z
New Revision: f15a790fd383665ec4defa0711e975476fd8b18b
URL:
https://github.com/llvm/llvm-project/commit/f15a790fd383665ec4defa0711e975476fd8b18b
DIFF:
https://github.com/llvm/llvm-project/commit/f15a790fd383665ec4defa0711e975476fd8b18b.diff
dwblaikie wrote:
Cool cool - thanks for the extra context!
https://github.com/llvm/llvm-project/pull/84423
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/dwblaikie approved this pull request.
Sounds OK to me.
(though several other instances of system_category are touched in this patch -
out of curiosity, how are those different from the one fixed case you called
out in JSONTransport.cpp? Are the other uses of system_category
https://github.com/dwblaikie approved this pull request.
Yeah, still not sure why something like `%clang -### -c -target x86_64 -g
-gsplit-dwarf %s` wouldn't change behavior on a darwin host and cause the check
for gnu-pubnames to fail there but if you say that works, guess that's OK &
if
1 - 100 of 1310 matches
Mail list logo
