[clang] [clang-tools-extra] [flang] [lld] [lldb] [llvm] [NFC] Rename Option parsing related files from OptXYZ -> OptionXYZ (PR #110692)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Yeah, I think given how wide the change is and the speculative nature of the 
harm - probably best to abandon this for now. We can pick it up again if more 
folks speak up/more evidence is presented that it's problematic.

https://github.com/llvm/llvm-project/pull/110692
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Add "extend lifetime" flags and release note (PR #110000)

[David Blaikie via cfe-commits]

@@ -211,6 +211,16 @@ New Compiler Flags
   only for thread-local variables, and none (which corresponds to the
   existing ``-fno-c++-static-destructors`` flag) skips all static
   destructors registration.
+- The ``-fextend-lifetimes`` and ``-fextend-this-ptr`` flags have been added to
+  allow for improved debugging of optimized code. Using ``-fextend-lifetimes``
+  will cause Clang to generate code that tries to preserve the lifetimes of
+  source variables, meaning that variables will typically be visible in a
+  debugger more often. The ``-fextend-this-ptr`` flag has the same behaviour,
+  but applies only to the ``this`` variable in C++ class member functions. Note
+  that this flag modifies the optimizations that Clang performs, which will
+  result in reduced performance in generated code; however, this feature will
+  not extend the lifetime of some variables in cases where doing so would have

dwblaikie wrote:

huh - what cases is this alluding to?

https://github.com/llvm/llvm-project/pull/11
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Add "extend lifetime" flags and release note (PR #110000)

[David Blaikie via cfe-commits]

@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -emit-llvm -O2 -fextend-lifetimes -o - | FileCheck 
--check-prefixes=CHECK-ALL,CHECK-O2 %s
+// RUN: %clang_cc1 %s -emit-llvm -O0 -fextend-lifetimes -o - | FileCheck 
--check-prefixes=CHECK-ALL,CHECK-O0 %s
+
+// Checks that we emit the function attribute has_fake_uses when
+// -fextend-lifetimes is on and optimizations are enabled, and that it does not
+// when optimizations are disabled.
+
+// CHECK-ALL:define {{.*}}void @foo
+// CHECK-O2: attributes #0 = {{{.*}}has_fake_uses

dwblaikie wrote:

Interesting model - I haven't reviewed the other patches in this direction, but 
I'm a bit surprised this is the only IR effect from the frontend.

I take it this attribute is added, which then causes some middle end pass to 
add the fake uses themselves?

That seems a bit quirky in terms of creating an attribute to communicate that - 
I guess other directions have already been considered/found unsuitable? (a 
couple of others that come to mind include: having the frontend create the fake 
uses (limits reusability for other front ends) or having the frontend choose to 
insert the fake use pass or not (and then the pass is unconditional/doesn't 
look for an attribute) - I /think/ a combination of these might be how the 
sanitizers work? (frontend chooses to include certain passes but might also 
have to do some different IRGen too))

https://github.com/llvm/llvm-project/pull/11
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Add "extend lifetime" flags and release note (PR #110000)

[David Blaikie via cfe-commits]

@@ -2217,6 +2217,11 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions 
&Opts, ArgList &Args,
   Args.getAllArgValues(OPT_fsanitize_trap_EQ), Diags,
   Opts.SanitizeTrap);
 
+  Opts.ExtendThisPtr =
+  Opts.OptimizationLevel > 0 && Args.hasArg(OPT_fextend_this_ptr);

dwblaikie wrote:

Is it worth maybe making this inconditional, even at -O0, in case someone's put 
an optimize attribute on a function, etc, and wants to override it?

https://github.com/llvm/llvm-project/pull/11
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][DebugInfo] Revert to printing canonical typenames for template aliases (PR #110767)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Thanks @jimingham - yeah, that makes sense. My original argument for being OK 
with the de-canonicalization of typedefs was that they aren't load bearing 
anyway. But if lldb puts load on them, that assumption doesn't hold.

Yeah, could check for angles or template parameter DIEs and cache 
typedef->prettyprinter only if none of those features appear.

https://github.com/llvm/llvm-project/pull/110767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][DebugInfo] Revert to printing canonical typenames for template aliases (PR #110767)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Not quite sure how we get to caching and template specializations and pretty 
printers.

If we're still producing the typedef-style DWARF for these alias template 
specializations - perhaps lldb could not cache pretty printers for typedefs? (I 
guess the pretty printers shouldn't be typedef-specific, right, since typedefs 
are transparent anyway - but I guess maybe pretty printers can be 
typedef-specific because typedefs can be intended to communicate what kind of a 
thing is and possibly how to render it?) - it'll cache teh pretty printer for 
the underlying type anyway, yeah?

https://github.com/llvm/llvm-project/pull/110767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Correct the line attribution for IF branches (PR #108300)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie approved this pull request.

Fair enough

https://github.com/llvm/llvm-project/pull/108300
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Correct the line attribution for IF branches (PR #108300)

[David Blaikie via cfe-commits]

@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -gno-column-info 
-triple=x86_64-pc-linux -emit-llvm %s -o - | FileCheck  %s
+
+// The important thing is that the compare and the conditional branch have
+// locs with the same scope (the lexical block for the 'if'). By turning off
+// column info, they end up with the same !dbg record, which halves the number
+// of checks to verify the scope.
+
+int c = 2;
+
+int f() {
+#line 100
+  if (int a = 5; a > c)
+return 1;
+  return 0;
+}
+// CHECK-LABEL: define {{.*}} @_Z1fv()
+// CHECK:   = icmp {{.*}} !dbg [[F_CMP:![0-9]+]]
+// CHECK-NEXT:  br i1 {{.*}} !dbg [[F_CMP]]

dwblaikie wrote:

& I guess this all only applies when the control structure doesn't have `{}`? 
(that being why the test doesn't have braces?)

https://github.com/llvm/llvm-project/pull/108300
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Correct the line attribution for IF branches (PR #108300)

[David Blaikie via cfe-commits]

@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -gno-column-info 
-triple=x86_64-pc-linux -emit-llvm %s -o - | FileCheck  %s
+
+// The important thing is that the compare and the conditional branch have
+// locs with the same scope (the lexical block for the 'if'). By turning off
+// column info, they end up with the same !dbg record, which halves the number
+// of checks to verify the scope.
+
+int c = 2;
+
+int f() {
+#line 100
+  if (int a = 5; a > c)
+return 1;
+  return 0;
+}
+// CHECK-LABEL: define {{.*}} @_Z1fv()
+// CHECK:   = icmp {{.*}} !dbg [[F_CMP:![0-9]+]]
+// CHECK-NEXT:  br i1 {{.*}} !dbg [[F_CMP]]

dwblaikie wrote:

Hmm, yeah, that `EmitStopPoint` seems a bit unstable/unreliable - the scoped 
location handling is designed to be more robust to ensure locations don't "leak 
out" beyond where they're meant to apply... 

I think maybe `EmitStopPoint` should be removed/reconsidered, but that's 
perhaps beyond the scope (har har) of this issue - but thoughts in case anyone 
else feels like picking up and running with that.

How's this location compare to other control structures (loops, etc) - do we (& 
GCC) use the condition as the location for the branch instructions, or would it 
be more suitable to use the start of `if` itself?

https://github.com/llvm/llvm-project/pull/108300
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Correct the line attribution for IF branches (PR #108300)

[David Blaikie via cfe-commits]

@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -gno-column-info 
-triple=x86_64-pc-linux -emit-llvm %s -o - | FileCheck  %s
+
+// The important thing is that the compare and the conditional branch have
+// locs with the same scope (the lexical block for the 'if'). By turning off
+// column info, they end up with the same !dbg record, which halves the number
+// of checks to verify the scope.
+
+int c = 2;
+
+int f() {
+#line 100
+  if (int a = 5; a > c)
+return 1;
+  return 0;
+}
+// CHECK-LABEL: define {{.*}} @_Z1fv()
+// CHECK:   = icmp {{.*}} !dbg [[F_CMP:![0-9]+]]
+// CHECK-NEXT:  br i1 {{.*}} !dbg [[F_CMP]]

dwblaikie wrote:

How come this case ^ already works (so far as I can tell) without this patch, 
but the others don't? Because we introduce a scope for the variable declaration 
in a way that we don't in the (cond) case?

https://github.com/llvm/llvm-project/pull/108300
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][Sema] Fix diagnostic for function overloading in extern "C" (PR #106033)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie approved this pull request.


https://github.com/llvm/llvm-project/pull/106033
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][Sema] Fix diagnostic for function overloading in extern "C" (PR #106033)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> This make sense to me, @dwblaikie wdyt?

Seems OK to me

https://github.com/llvm/llvm-project/pull/106033
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Assert non-null enum definition in CGDebugInfo::CreateTypeDefinition(const EnumType*) (PR #105556)

[David Blaikie via cfe-commits]

dwblaikie wrote:

What static analysis tool flagged this? Any idea why this particular null 
dereference, when LLVM/Clang have loads of assumed-non-null pointers that are 
dereferenced in a great many places?

https://github.com/llvm/llvm-project/pull/105556
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> Looking forward to trying this out!
> 
> Should the new flag have some docs and maybe be mentioned in the release 
> notes, or do we think it's not ready for prime time yet for some reason?

I'm /pretty/ neutral on that - it's got pretty clear behavior, etc (& in fact 
some class members already had this handling -implicit special members, member 
templates, and nested types) - but I expect the ecosystem (debuggers - gdb and 
lldb) won't work well with this.

It's not too hard to observe GDB showing a version of a different type 
depending on the context you're in - so name resolution will fail depending on 
which copy of the type the debugger is using at the time. LLDB I expect will 
fail even harder, because it creates fewer copies of a type - -so it'll pick 
one seemingly at random and only give you that view, even in some other context 
where GDB would give the type view local to that context at least, I think.

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

[clang] [lld] [llvm] [mlir] [NFC][IWYU] Update Support library with IWYU. (PR #102707)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> > The motivation is as usual IWYU and similar refactoring - to reduce build 
> > time and probablility of non-related source(s) recompile.
> 
> I'm confused: as far as I know IWYU achieves the opposite of what you're 
> describing actually: it adds more includes than strictly necessary. (IIUC, 
> the motivation for IWYU is rather correctness and robustness to refactoring 
> and changes).

It can do both - it can remove an unused header, and add a used header that 
might be indirectly depended on. (& can switch an include to a forward 
declaration - or the opposite (if a forward declaration is used, but a 
definition is required and is currently provided indirectly through some other 
#include))

> > Yes, but I actually do not see what part of the mentioned standard' section 
> > conflicts with the change. Would you please suggest an example where we see 
> > a situation when applied IWYU approach can contradict with the part of 
> > Coding Standards?
> 
> I haven't sanity checked your patch: IWYU is just known to historically to 
> the opposite of what we're trying to do in LLVM (as in: "include strictly the 
> minimum and rely on forward declarations"), either the tools was updated to 
> support the LLVM style, or you've been doing a lot of manual work (but then 
> the PR title is misleading).

FWIW, IWYU does do forward declarations ( 
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/WhyIWYU.md#why-forward-declare
 )

https://github.com/llvm/llvm-project/pull/102707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [Clang] Protect ObjCMethodList assignment operator against self-assignment (PR #97933)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Is there existing test coverage (like, if you add an assertion, instead of an 
if/return - do any tests hit the assertion)? or could you add some?

https://github.com/llvm/llvm-project/pull/97933
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][CGDebugInfo] Don't generate an implicit 'this' parameter if one was specified explicitly (PR #100767)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/100767
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 8dd5742 - Fix aarch64-ptrauth.c to avoid writing to cwd which might not be writeable

[David Blaikie via cfe-commits]

Author: David Blaikie
Date: 2024-07-25T21:52:14Z
New Revision: 8dd574236ccaa0a183278396cfec3068b832651a

URL: 
https://github.com/llvm/llvm-project/commit/8dd574236ccaa0a183278396cfec3068b832651a
DIFF: 
https://github.com/llvm/llvm-project/commit/8dd574236ccaa0a183278396cfec3068b832651a.diff

LOG: Fix aarch64-ptrauth.c to avoid writing to cwd which might not be writeable

Some of the tests seem to test beyond the driver (& check a warning
coming from the frontend) and should probably be split into separate
tests.

Added: 


Modified: 
clang/test/Driver/aarch64-ptrauth.c

Removed: 




diff  --git a/clang/test/Driver/aarch64-ptrauth.c 
b/clang/test/Driver/aarch64-ptrauth.c
index c8e3aeef1640a..75190c4380826 100644
--- a/clang/test/Driver/aarch64-ptrauth.c
+++ b/clang/test/Driver/aarch64-ptrauth.c
@@ -49,14 +49,14 @@
 // ERR1-NEXT: error: unsupported option '-fptrauth-init-fini' for target 
'{{.*}}'
 
  Only support PAuth ABI for Linux as for now.
-// RUN: not %clang -c --target=aarch64-unknown -mabi=pauthtest %s 2>&1 | 
FileCheck %s --check-prefix=ERR2
-// RUN: not %clang -c --target=aarch64-unknown-pauthtest   %s 2>&1 | 
FileCheck %s --check-prefix=ERR2
+// RUN: not %clang -o /dev/null -c --target=aarch64-unknown -mabi=pauthtest %s 
2>&1 | FileCheck %s --check-prefix=ERR2
+// RUN: not %clang -o /dev/null -c --target=aarch64-unknown-pauthtest   %s 
2>&1 | FileCheck %s --check-prefix=ERR2
 // ERR2: error: ABI 'pauthtest' is not supported for 
'aarch64-unknown-unknown-pauthtest'
 
  PAuth ABI is encoded as environment part of the triple, so don't allow to 
explicitly set other environments.
-// RUN: not %clang -c --target=aarch64-linux-gnu -mabi=pauthtest %s 2>&1 | 
FileCheck %s --check-prefix=ERR3
+// RUN: not %clang -### -c --target=aarch64-linux-gnu -mabi=pauthtest %s 2>&1 
| FileCheck %s --check-prefix=ERR3
 // ERR3: error: unsupported option '-mabi=pauthtest' for target 
'aarch64-unknown-linux-gnu'
-// RUN: %clang -c --target=aarch64-linux-pauthtest -mabi=pauthtest %s
+// RUN: %clang -### -c --target=aarch64-linux-pauthtest -mabi=pauthtest %s
 
  The only branch protection option compatible with PAuthABI is BTI.
 // RUN: not %clang -### -c --target=aarch64-linux -mabi=pauthtest 
-mbranch-protection=pac-ret %s 2>&1 | \



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Fix null pointer dereference in enum debug info generation (PR #97105)

[David Blaikie via cfe-commits]

@@ -98,3 +98,6 @@ enum E8 { A8 = -128, B8 = 127 } x8;
 // CHECK-NOT: DIFlagEnumClass
 // CHECK: !DIEnumerator(name: "A8", value: -128)
 
+// Forward declaration of an enum class.
+enum class Color : int;
+// CHECK-NOT: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Color"

dwblaikie wrote:

Have you run this test? Because an unused enum like this usually 
isn't/shouldn't be emitted into the output... - note all the other enums have a 
variable declared of their type (the trailing "} xN;")

https://github.com/llvm/llvm-project/pull/97105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Revert "[PS4/PS5][Driver][DWARF] Always emit .debug_aranges for SCE t… (PR #99711)

[David Blaikie via cfe-commits]

dwblaikie wrote:

also, if you're sending a pull request just because it's convenient (via the 
web UI "revert" button) or to run presubmits but not for review, please include 
the skip-presubmit-approval label so it's clear something isn't getting 
committed without needed review

https://github.com/llvm/llvm-project/pull/99711
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Revert "Add source file name for template instantiations in -ftime-trace" (PR #99534)

[David Blaikie via cfe-commits]

dwblaikie wrote:

(if you don't need a code review for a change, you can commit it directly 
without a pull request - if you're making a PR not for code review but for 
presubmit checks, please label to `skip-precommit-approval` to clarify that you 
aren't waiting for approval (and that the change then wasn't submitted without 
the intended/required approval))

https://github.com/llvm/llvm-project/pull/99534
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> > That's a pretty substantial policy change to propose, and this probably 
> > isn't the place to propose/discuss it. If that's your intent, probably best 
> > to take that up on discord.
> 
> I am not proposing a policy change. I believe the current policy is aimed at 
> giving an escape hatch for projects which there is basically one or two 
> active developers. I am pointing out that I believe we don't need for that 
> escape hatch to apply to any parts of clang currently.

With a fair bit of experience on this project and the norms - I can say pretty 
confidently that was/is not the intent of the current policy. Especially for 
code owners, there's an expectation they probably know better than anyone else 
& can make summary judgments on direction in many cases. Every now and then 
they'll ask for second opinions on uncertain directions, but can probably 
pretty well determine what's acceptable for most work without a second opinion.

(honestly, moving to all changes being reviewed might be good for the project - 
though I think we'd need to work more on ownership and reviewer 
availability/authority/process, but it's not where we are today in either case 
(policy or socially))

The dev policy says "Smaller patches (or patches where the developer owns the 
component) that meet likely-community-consensus requirements (as apply to all 
patch approvals) can be committed prior to an explicit review. " (in this case 
@ChuanqiXu9 is the owner of clang's modules support)

> > (FWIW, check some of the recent modules changes @ChuanqiXu9 has been 
> > working on to see that reviewer bandwidth here is pretty thin (& my 
> > experience in LLVM in general, including clang, is that reviewer bandwidth 
> > is pretty thin - though it is something we should address & I do think it 
> > might be time to change LLVM's post-commit review policy, but I think it'll 
> > be a substantial amount of work))
> 
> If you feel bandwidth for modules is pretty thin, I put myself available as a 
> reviewer, so feel free to ping me. I may not have a lot of time available for 
> fully reviewing big patches, but I can certainly help with the smaller 
> patches such as this one.

While I appreciate the offer and would encourage you to review these changes - 
I'll point out that one of the reasons these reviews haven't been forthcoming 
is that the feature area is fairly complicated and nuanced.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Fix null pointer dereference in enum debug info generation (PR #97105)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> Thanks @smanna12. I think this looks ok; returning null here does appear to 
> be consistent with other overloads of `CreateTypeDefinition` for `RecordType` 
> and `ObjCInterfaceType`. I agree with @Michael137 that it would be nice to 
> have an example that fails the added condition. The code already checks for 
> an incomplete type so is presumably intended to handle such types. Perhaps we 
> are missing a test though? Presumably one that uses a forward declareable 
> enum type?

Not a hard line - but I'd discourage approving a patch like this until the test 
issue has been resolved (either a test is added, or a good explanation for it 
being omitted has been documented).

https://github.com/llvm/llvm-project/pull/97105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> Also, I think we presently have sufficient review bandwidth in clang proper, 
> even including the area of modules, so that any non-trivial change can be 
> reviewed, and I also propose that we abstain from trying to directly commit 
> those.

That's a pretty substantial policy change to propose, and this probably isn't 
the place to propose/discuss it. If that's your intent, probably best to take 
that up on discord.

NFC, to me, has always been basically "this has no observable change/can't be 
tested" - it's not a bar on whether something should be reviewed. (lots of NFC 
patches get sent for review, but not all of them to be sure - and equally not 
all non-NFC patches get sent for review)

(FWIW, check some of the recent modules changes @ChuanqiXu9 has been working on 
to see that reviewer bandwidth here is pretty thin (& my experience in LLVM in 
general, including clang, is that reviewer bandwidth is pretty thin - though it 
is something we should address & I do think it might be time to change LLVM's 
post-commit review policy, but I think it'll be a substantial amount of work))

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> I think the appropriate tag for such commits would be NFCI, and should still 
> require PR and review.

Clarifying a couple of things here
1) I tend to agree the patch might've been a bit subtle and maybe split up into 
smaller, independent parts (usual rules: [be an isolated change. Independent 
changes should be submitted as separate patches as this makes reviewing 
easier](https://llvm.org/docs/Contributing.html#how-to-submit-a-patch)
2) I really don't think anyone should read into a distinction between NFC and 
NFCI, I don't think it's helpful to try to create a distinction here - NFC is 
already a statement of intent, as much as any patch description is always a 
statement of intent. We all make mistakes, but I don't think adding "intended" 
to the acronym is really helpful
3) Non-NFC changes don't necessarily require PR/review - LLVM allows for, 
especially code owners, to commit without precommit review if the change is 
sufficiently obvious. So some simple cleanups, clear bug fixes, direction 
already has buy-in, etc, are often committed without precommit review.

But, yeah, bit more caution in the future, and probably smaller/more isolated 
patches would be good in any case.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

@@ -185,6 +203,18 @@ CALL_AO(PackedMembers)
 // CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 1 {{.*}} align 1 
{{.*}}i64 16, i1 {{.*}})
 // CHECK: ret ptr
 
+// WithEmptyField copy-assignment:
+// CHECK-LABEL: define linkonce_odr nonnull align {{[0-9]+}} 
dereferenceable({{[0-9]+}}) ptr @_ZN14WithEmptyFieldaSERKS_
+// CHECK: call void @llvm.memcpy.p0.p0.i64({{.*}} align 4 {{.*}} align 4 
{{.*}}i64 4, i1 {{.*}})

dwblaikie wrote:

Sorry, jumping in here without a lot of context, but the change in 
`%struct.WithEmptyField` seems wrong, doesn't it? Like that is a change in 
layout/ABI break/bad thing? I guess I'm missing something about why that change 
in layout would be acceptable? (maybe padding - if NonPOD gets aligned, such 
that the structure isn't changing layout after all?)

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

[clang] [Clang][Sema] Diagnose variable template explicit specializations with storage-class-specifiers (PR #93873)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> It looks like the presence of `static` on template variable specializations 
> makes difference in the namespace context: https://gcc.godbolt.org/z/WGsreqbz8
> 
> Specifically, the specializations not marked `static` result in an exported 
> variable. Thus, we have seemingly valid code that generates this diagnostic:
> 
> ```
> namespace A {
> template 
> static constexpr unsigned kMaxUnsignedInt = 2 * kMaxUnsignedInt + 1;
> 
> template <>
> static constexpr unsigned kMaxUnsignedInt<1> = 1;
> }
> ```
> 
> However, if we remove the `static` from the specialization, we're getting a 
> potential ODR violation (considering this code can be in a header included 
> into multiple translation units).
> 
> The right thing for this specific case is `inline constexpr`, but 
> nevertheless, there seems to be an inconsistency now in how Clang handles the 
> case now.

This seems to be pretty clearly a bug - in clang's specialization handling even 
prior to this patch.
GCC doesn't do this, it uses the storage class of the generic template for the 
specialization, which seems correct to me.

@sdkrystian are you interested in/able to fix this ^ ? 
https://gcc.godbolt.org/z/oPxKvvdeM


https://github.com/llvm/llvm-project/pull/93873
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Driver] Add -Wa, options --crel and --allow-experimental-crel (PR #97378)

[David Blaikie via cfe-commits]

@@ -0,0 +1,25 @@
+// RUN: not %clang -### -c --target=x86_64 -Wa,--crel %s 2>&1 | FileCheck %s 
--check-prefix=NOEXP
+
+// NOEXP: error: -Wa,--allow-experimental-crel must be specified to use 
-Wa,--crel. CREL is experimental and takes a non-standard section type code
+
+// RUN: %clang -### -c --target=x86_64 -Wa,--crel,--allow-experimental-crel %s 
-Werror 2>&1 | FileCheck %s
+// RUN: %clang -### -c --target=x86_64 
-Wa,--crel,--no-crel,--allow-experimental-crel %s -Werror 2>&1 | FileCheck %s 
--check-prefix=NO
+// RUN: %clang -### -c --target=x86_64 -Wa,--allow-experimental-crel %s 
-Werror 2>&1 | FileCheck %s --check-prefix=NO
+// RUN: not %clang -### -c --target=arm64-apple-darwin 
-Wa,--crel,--allow-experimental-crel %s 2>&1 | FileCheck %s --check-prefix=ERR
+// RUN: not %clang -### -c --target=mips64 
-Wa,--crel,--allow-experimental-crel %s 2>&1 | FileCheck %s --check-prefix=ERR
+
+// RUN: %clang -### -c --target=aarch64 -Werror 
-Wa,--crel,--allow-experimental-crel -x assembler %s -Werror 2>&1 | FileCheck 
%s --check-prefix=ASM
+// RUN: not %clang -### -c --target=mips64 
-Wa,--crel,--allow-experimental-crel -x assembler %s 2>&1 | FileCheck %s 
--check-prefix=ERR
+
+// CHECK: "-cc1" {{.*}}"--crel"
+// NO: "-cc1"
+// NO-NOT: "--crel"
+// ASM:   "-cc1as" {{.*}}"--crel"
+// ERR: error: unsupported option '-Wa,--crel' for target '{{.*}}'
+
+/// Don't bother with --allow-experimental-crel for LTO.

dwblaikie wrote:

"is unlikely to be desirable for LTO"? (crel is never required)

https://github.com/llvm/llvm-project/pull/97378
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Use std::make_unique (NFC) (PR #97176)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie approved this pull request.


https://github.com/llvm/llvm-project/pull/97176
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Use std::make_unique (NFC) (PR #97176)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie edited 
https://github.com/llvm/llvm-project/pull/97176
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Clang: Add warning flag for storage class specifiers on explicit specializations (PR #96699)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie closed 
https://github.com/llvm/llvm-project/pull/96699
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Clang: Add warning flag for storage class specifiers on explicit specializations (PR #96699)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> Minor nit: Probably don't need to explicitly define a diagnostic group, since 
> it's only ever used for this diagnostic.

Not sure what the conventions are - but I rather like the normalization of 
having the explicit group, better chance of finding the group/reusing it maybe? 
So i'll leave it there for now, but if you feel strongly/other folks chime in - 
happy to change it in a follow-up commit. (& good to understand that explicit 
groups aren't necessary/the inline group definition is a valid option, though)

https://github.com/llvm/llvm-project/pull/96699
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Static and explicit object member functions with the same parameter-type-lists (PR #93430)

[David Blaikie via cfe-commits]

dwblaikie wrote:

@cor3ntin ping on this?

https://github.com/llvm/llvm-project/pull/93430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Oh, this code was touched really recently in 
66df6141659375e738d9b9b74bf79b2317576042 (@augusto2112 ) (see notes in previous 
comments about how this code should be generalized)

https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Here's the smallest patch that would put explicit alignment on any packed 
structure:
```
diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp 
b/clang/lib/CodeGen/CGDebugInfo.cpp
index a072475ba770..bbb13ddd593b 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -64,7 +64,7 @@ static uint32_t getTypeAlignIfRequired(const Type *Ty, const 
ASTContext &Ctx) {
   // MaxFieldAlignmentAttr is the attribute added to types
   // declared after #pragma pack(n).
   if (auto *Decl = Ty->getAsRecordDecl())
-if (Decl->hasAttr())
+if (Decl->hasAttr() || Decl->hasAttr())
   return TI.Align;
 
   return 0;
```

But I don't think that's the right approach - I think what we should do is 
compute the natural alignment of the structure, then compare that to the actual 
alignment - and if they differ, we should put an explicit alignment on the 
structure. This avoids the risk that other alignment-influencing effects might 
be missed (and avoids the case of putting alignment on a structure that, when 
packed, just has the same alignment anyway - which is a minor issue, but nice 
to get right (eg: packed struct of a single char probably shouldn't have an 
explicit alignment - since it's the same as the implicit alignment anyway))



https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

dwblaikie wrote:

The other effects of `packed` are encoded (the changes to the member offsets) 
but that's not the only effect, and we shouldn't use that effect (which isn't 
guaranteed to be observable - as in the case of "packed struct with a single 
member/that just happens to not have padding anyway") to try to divine the 
alignment.

https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

dwblaikie wrote:

oh, slight misquote above - `aligned(1)` is a no-op, as that attribute can't 
reduce the alignment... 

But I think my argument still stands, roughly as - if increases in alignment 
are explicitly specified, decreases in alignment should be too.

https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Ah, I think this right solution to /this/ case is that the compiler should be 
emitting the alignment for the structure?

Like there's no way for the debugger to know that this structure is 
underaligned at the moment:
```
struct __attribute__((packed)) t1 {
  int i;
};
```
Which means the debugger might generate code that assumes `t1` is naturally 
aligned, and break (ARM crashes on underaligned operations, doesn't it? so if 
you got a pointer to t1 back from some API that happened to have put it in an 
underaligned location - then in your expression you tried to read `i`, this 
could generate crashing code?)

Essentially the `packed` attribute on the above structure is providing the same 
value/effect as `aligned(1)` and should produce the same DWARF, but it doesn't.

https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Clang: Add warning flag for storage class specifiers on explicit specializations (PR #96699)

[David Blaikie via cfe-commits]

dwblaikie wrote:

> Should we add a test for this flag? Something around existing tests in 
> clang/test/CXX/temp/temp.spec/temp.expl.spec/p2-20.cpp and 
> clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp.

Oh, right, I did have a new test I meant to add, but that's a better place for 
it - so I've added that test coverage in 
https://github.com/llvm/llvm-project/pull/96699/commits/b3facc9dffc762e3cef93886df23026d502afcae

(I'm out for the day, so if anyone happens to approve this and feel like 
pressing the "squash and merge" button that'd be swell, otherwise I'll get to 
it when I have a chance/tomorrow)

https://github.com/llvm/llvm-project/pull/96699
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Clang: Add warning flag for storage class specifiers on explicit specializations (PR #96699)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie updated 
https://github.com/llvm/llvm-project/pull/96699

>From a539afc7b81502ffcab7028bfe8266b8e32951d9 Mon Sep 17 00:00:00 2001
From: David Blaikie 
Date: Tue, 25 Jun 2024 21:02:50 +
Subject: [PATCH 1/2] Clang: Add warning flag for storage class specifiers on
 explicit specializations

With the recent fix for this situation in class members (#93873) (for
which the fixed code is invalid prior to this patch - making migrating
code difficult as it must be in lock-step with the compiler migration,
if building with -Werror) it'd be really useful to be able to disable
this warning during the compiler migration/decouple the compiler
migration from the source fixes.

In theory this approach will regress the codebase to the previous
non-member cases of this issue that were already being held back by the
warning (as opposed to if we carved out the new cases into a separate
warning from the existing cases) but I think this'll be so rare and the
cleanup so simple, that the extra regressions of disabling the warning
broadly won't be too much of a problem. (but if folks disagree, I'm open
to making the warning more fine-grained)
---
 clang/include/clang/Basic/DiagnosticGroups.td| 4 
 clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 +-
 clang/test/Misc/warning-flags.c  | 3 +--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 9b37d4bd3205b..8c5cbbc160ba1 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1535,3 +1535,7 @@ def BitIntExtension : DiagGroup<"bit-int-extension">;
 
 // Warnings about misuse of ExtractAPI options.
 def ExtractAPIMisuse : DiagGroup<"extractapi-misuse">;
+
+// Warnings about using the non-standard extension having an explicit 
specialization
+// with a storage class specifier.
+def ExplicitSpecializationStorageClass : 
DiagGroup<"explicit-specialization-storage-class">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 25a87078a5709..f12121f065783 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5366,7 +5366,7 @@ def err_not_class_template_specialization : Error<
   "cannot specialize a %select{dependent template|template template "
   "parameter}0">;
 def ext_explicit_specialization_storage_class : ExtWarn<
-  "explicit specialization cannot have a storage class">;
+  "explicit specialization cannot have a storage class">, 
InGroup;
 def err_dependent_function_template_spec_no_match : Error<
   "no candidate function template was found for dependent"
   " %select{member|friend}0 function template specialization">;
diff --git a/clang/test/Misc/warning-flags.c b/clang/test/Misc/warning-flags.c
index dd73331913c6f..ae14720959a0e 100644
--- a/clang/test/Misc/warning-flags.c
+++ b/clang/test/Misc/warning-flags.c
@@ -18,10 +18,9 @@ This test serves two purposes:
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (66):
+CHECK: Warnings without flags (65):
 
 CHECK-NEXT:   ext_expected_semi_decl_list
-CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex

>From b3facc9dffc762e3cef93886df23026d502afcae Mon Sep 17 00:00:00 2001
From: David Blaikie 
Date: Wed, 26 Jun 2024 00:01:46 +
Subject: [PATCH 2/2] Add test

---
 clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp | 16 
 ...n-explicit-specialization-storage-class.cpp | 18 ++
 2 files changed, 30 insertions(+), 4 deletions(-)
 create mode 100644 
clang/test/SemaCXX/warn-explicit-specialization-storage-class.cpp

diff --git a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp 
b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
index f6b5d2487e73d..9efa7b67f5bdb 100644
--- a/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.spec/dcl.stc/p1.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify=expected,spec %s
+// RUN: %clang_cc1 -fsyntax-only -verify 
-Wno-explicit-specialization-storage-class %s
 
 // A storage-class-specifier shall not be specified in an explicit
 // specialization (14.7.3) or an explicit instantiation (14.7.2)
@@ -7,13 +8,13 @@ template void f(T) {}
 template static void g(T) {}
 
 
-template<> static void f(int); // expected-warning{{explicit 
specialization cannot have a storage class}}
+template<> static void f(int); // spec-warning{{explicit specialization 
cannot have a storage class}}
 template static void f(float); // expected-error{{explicit 
instantiation cannot have a storage class}}
 
 template<> void f(double);
 template void f(long);
 
-templ

[clang] [llvm] [MC,llvm-readobj,yaml2obj] Support CREL relocation format (PR #91280)

[David Blaikie via cfe-commits]

@@ -86,6 +86,8 @@ DYNAMIC_TAG(RELRSZ, 35)  // Size of Relr relocation table.
 DYNAMIC_TAG(RELR, 36)// Address of relocation table (Relr entries).
 DYNAMIC_TAG(RELRENT, 37) // Size of a Relr relocation entry.
 
+DYNAMIC_TAG(CREL,  38)   // CREL relocation table
+

dwblaikie wrote:

> I think we need to avoid generic numbers for dynamic tags/section types etc, 
> except possibly generic numbers that are a long way from the "normal" range. 

+1.

I've tried to pushback on using any standard range before it's been 
standardized (eg: 
https://github.com/llvm/llvm-project/pull/91280#issuecomment-2099201183 
https://github.com/llvm/llvm-project/pull/91280#issuecomment-2101019352 ) but I 
haven't looked through the change completely to see all the enumerations used 
and how they're reserved, whether they use an extension space or what the 
argument is for them not using one.

Perhaps someone could summarize this aspect of the patch?

https://github.com/llvm/llvm-project/pull/91280
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Sema] Diagnose variable template explicit specializations with storage-class-specifiers (PR #93873)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Sent a patch to add a warning flag for the warning this patch uses: 
https://github.com/llvm/llvm-project/pull/96699

With that, we could disable the warning during the compiler migration, 
decoupling compiler migration from code cleanup.

https://github.com/llvm/llvm-project/pull/93873
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Clang: Add warning flag for storage class specifiers on explicit specializations (PR #96699)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie created 
https://github.com/llvm/llvm-project/pull/96699

With the recent fix for this situation in class members (#93873) (for
which the fixed code is invalid prior to this patch - making migrating
code difficult as it must be in lock-step with the compiler migration,
if building with -Werror) it'd be really useful to be able to disable
this warning during the compiler migration/decouple the compiler
migration from the source fixes.

In theory this approach will regress the codebase to the previous
non-member cases of this issue that were already being held back by the
warning (as opposed to if we carved out the new cases into a separate
warning from the existing cases) but I think this'll be so rare and the
cleanup so simple, that the extra regressions of disabling the warning
broadly won't be too much of a problem. (but if folks disagree, I'm open
to making the warning more fine-grained)

>From a539afc7b81502ffcab7028bfe8266b8e32951d9 Mon Sep 17 00:00:00 2001
From: David Blaikie 
Date: Tue, 25 Jun 2024 21:02:50 +
Subject: [PATCH] Clang: Add warning flag for storage class specifiers on
 explicit specializations

With the recent fix for this situation in class members (#93873) (for
which the fixed code is invalid prior to this patch - making migrating
code difficult as it must be in lock-step with the compiler migration,
if building with -Werror) it'd be really useful to be able to disable
this warning during the compiler migration/decouple the compiler
migration from the source fixes.

In theory this approach will regress the codebase to the previous
non-member cases of this issue that were already being held back by the
warning (as opposed to if we carved out the new cases into a separate
warning from the existing cases) but I think this'll be so rare and the
cleanup so simple, that the extra regressions of disabling the warning
broadly won't be too much of a problem. (but if folks disagree, I'm open
to making the warning more fine-grained)
---
 clang/include/clang/Basic/DiagnosticGroups.td| 4 
 clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 +-
 clang/test/Misc/warning-flags.c  | 3 +--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 9b37d4bd3205b..8c5cbbc160ba1 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1535,3 +1535,7 @@ def BitIntExtension : DiagGroup<"bit-int-extension">;
 
 // Warnings about misuse of ExtractAPI options.
 def ExtractAPIMisuse : DiagGroup<"extractapi-misuse">;
+
+// Warnings about using the non-standard extension having an explicit 
specialization
+// with a storage class specifier.
+def ExplicitSpecializationStorageClass : 
DiagGroup<"explicit-specialization-storage-class">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 25a87078a5709..f12121f065783 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5366,7 +5366,7 @@ def err_not_class_template_specialization : Error<
   "cannot specialize a %select{dependent template|template template "
   "parameter}0">;
 def ext_explicit_specialization_storage_class : ExtWarn<
-  "explicit specialization cannot have a storage class">;
+  "explicit specialization cannot have a storage class">, 
InGroup;
 def err_dependent_function_template_spec_no_match : Error<
   "no candidate function template was found for dependent"
   " %select{member|friend}0 function template specialization">;
diff --git a/clang/test/Misc/warning-flags.c b/clang/test/Misc/warning-flags.c
index dd73331913c6f..ae14720959a0e 100644
--- a/clang/test/Misc/warning-flags.c
+++ b/clang/test/Misc/warning-flags.c
@@ -18,10 +18,9 @@ This test serves two purposes:
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (66):
+CHECK: Warnings without flags (65):
 
 CHECK-NEXT:   ext_expected_semi_decl_list
-CHECK-NEXT:   ext_explicit_specialization_storage_class
 CHECK-NEXT:   ext_missing_whitespace_after_macro_name
 CHECK-NEXT:   ext_new_paren_array_nonconst
 CHECK-NEXT:   ext_plain_complex

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang] Static and explicit object member functions with the same parameter-type-lists (PR #93430)

[David Blaikie via cfe-commits]

dwblaikie wrote:

Hit an msan use-of-uninitialized on an internal use case (a tool that uses 
Clang via API).

the test was compiling this source code:
```
#include 
namespace proto2 {
struct Message {};
}  // namespace proto2
struct FakeProto : proto2::Message {
  const std::string& getter() const { return str; }  // NOLINT
  static std::string str;
};
struct S {
  S(std::string) {}  // NOLINT
};
void UseS(S s) {}
const std::string& foo() {
  FakeProto fp;
  return fp.getter();
}
void baz() { UseS(foo()); }
std::string s = foo();
```

and failed in this stack:
```
   #0 0x7f1da36404cd in NoteFunctionCandidate 
third_party/llvm/llvm-project/clang/lib/Sema/SemaOverload.cpp:12113:35
#1 0x7f1da36404cd in 
clang::OverloadCandidateSet::NoteCandidates(clang::Sema&, 
llvm::ArrayRef, llvm::ArrayRef, 
llvm::StringRef, clang::SourceLocation) 
third_party/llvm/llvm-project/clang/lib/Sema/SemaOverload.cpp:12733:7
#2 0x7f1da3207016 in clang::InitializationSequence::Diagnose(clang::Sema&, 
clang::InitializedEntity const&, clang::InitializationKind const&, 
llvm::ArrayRef) 
third_party/llvm/llvm-project/clang/lib/Sema/SemaInit.cpp:9933:26
#3 0x7f1da31fa0e2 in clang::InitializationSequence::Perform(clang::Sema&, 
clang::InitializedEntity const&, clang::InitializationKind const&, 
llvm::MutableArrayRef, clang::QualType*) 
third_party/llvm/llvm-project/clang/lib/Sema/SemaInit.cpp:8705:5
#4 0x7f1da2704fbe in clang::Sema::AddInitializerToDecl(clang::Decl*, 
clang::Expr*, bool) 
third_party/llvm/llvm-project/clang/lib/Sema/SemaDecl.cpp:13730:33
#5 0x7f1da58cfea0 in 
clang::Parser::ParseDeclarationAfterDeclaratorAndAttributes(clang::Declarator&, 
clang::Parser::ParsedTemplateInfo const&, clang::Parser::ForRangeInit*) 
third_party/llvm/llvm-project/clang/lib/Parse/ParseDecl.cpp:2787:17
#6 0x7f1da58c924b in clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, 
clang::DeclaratorContext, clang::ParsedAttributes&, 
clang::Parser::ParsedTemplateInfo&, clang::SourceLocation*, 
clang::Parser::ForRangeInit*) 
third_party/llvm/llvm-project/clang/lib/Parse/ParseDecl.cpp:2480:7
#7 0x7f1da5b06b4d in 
clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, 
clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) 
third_party/llvm/llvm-project/clang/lib/Parse/Parser.cpp:1249:10
#8 0x7f1da5b058b0 in 
clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, 
clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) 
third_party/llvm/llvm-project/clang/lib/Parse/Parser.cpp:1271:12
#9 0x7f1da5b0338b in 
clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, 
clang::ParsedAttributes&, clang::ParsingDeclSpec*) 
third_party/llvm/llvm-project/clang/lib/Parse/Parser.cpp:1074:14
#10 0x7f1da5afe984 in 
clang::Parser::ParseTopLevelDecl(clang::OpaquePtr&, 
clang::Sema::ModuleImportState&) 
third_party/llvm/llvm-project/clang/lib/Parse/Parser.cpp:763:12
#11 0x7f1da588c011 in clang::ParseAST(clang::Sema&, bool, bool) 
third_party/llvm/llvm-project/clang/lib/Parse/ParseAST.cpp:163:20
#12 0x7f1da743f407 in clang::ASTFrontendAction::ExecuteAction() 
third_party/llvm/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1192:3
#13 0x7f1da743dfab in clang::FrontendAction::Execute() 
third_party/llvm/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1078:8
```

I realize this isn't exactly repro steps - the  included is from 
libc++, for what that's worth - I don't /think/ this build is using modules.

But providing this info in case it's sufficient information to diagnose the 
issue.

It seems to be the TookAddressOfOverload variable is used-uninitialized 
somewhere in this path.

(I don't actually have a clang-built-with-msan to test this further myself at 
the moment, but trying to figure out some bootstrapping issues to try to help 
narrow it down)

https://github.com/llvm/llvm-project/pull/93430
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie approved this pull request.


https://github.com/llvm/llvm-project/pull/79230
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

@@ -28,7 +29,7 @@ namespace llvm {
 }
 
 // Prefix of the name of the artificial inline frame.
-#define CLANG_TRAP_PREFIX "__clang_trap_msg"
+inline constexpr llvm::StringRef CLANG_TRAP_PREFIX = "__clang_trap_msg";

dwblaikie wrote:

The name should be changed to use the usual variable/constant naming convention 
(ClangTrapPrefix, I think, in this case).

https://github.com/llvm/llvm-project/pull/79230
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

https://github.com/dwblaikie edited 
https://github.com/llvm/llvm-project/pull/79230
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Driver] Expose `-fno-eliminate-unused-debug-types` to clang-cl (PR #95259)

[David Blaikie via cfe-commits]

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.
> 
> Absolutely, this is only a short term hack until I figure out a better way to 
> fix it.
> 
> > Do you have a small example of where Clang and MSVC differ in emitting some 
> > particular unreferenced type? I would imagine MSVC isn't emitting /every/ 
> > written type...
> 
> It's essentially the examples in #46924. A class that is only used to hold 
> some const/constexpr values. These values are then used by the .NATVIS file. 
> With `/Z7`, MSVC seems to always emit them as `S_CONSTANT`s. But in Clang 
> since the type isn't used, it is never emitted by 
> `CGDebugInfo::EmitAndRetainType()`. Setting `DebugInfo == 
> llvm::codegenoptions::UnusedTypeInfo` fixes the problem. Is there a better 
> way?

Ah, I'll take up the discussion on the issue, I think - thanks for pointing 
that out.

https://github.com/llvm/llvm-project/pull/95259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

@@ -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

https://github.com/llvm/llvm-project/pull/79230
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Driver] Expose `-fno-eliminate-unused-debug-types` to clang-cl (PR #95259)

[David Blaikie via cfe-commits]

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 of where Clang and MSVC differ in emitting some 
particular unreferenced type? I would imagine MSVC isn't emitting /every/ 
written type... 

https://github.com/llvm/llvm-project/pull/95259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Clang][Driver] Expose `-fno-eliminate-unused-debug-types` to clang-cl (PR #95259)

[David Blaikie via cfe-commits]

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 correctly? 
https://godbolt.org/z/c1f6xKn3T

One way I've seen someone force the instantiation of the variable definition, 
and thus get the constant emitted into the DWARF, is using a static_assert:
```
template
struct t1 {
  static constexpr int x = 3 / Value;
  static_assert(x, true);
};
t1<1> v1;
```
(of course, in this case, `t1<0>` can no longer be instantiated)

https://github.com/llvm/llvm-project/pull/95259
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [BPF] Fix linking issues in static map initializers (PR #91310)

[David Blaikie via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

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 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).

The struct layout validation stuff that @Michael137 found may be adequate to 
provide confidence (especially combined with fuzzing, maybe) without the need 
for the codegen-is-zero-length-independent invariant.

I don't feel too strongly - mostly happy with whatever Clang owners are happy 
with.

https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

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 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.

(sorry if I'm jumping in without enough context... ) - couldn't the inverse be 
true, then - that codegen should ignore if something `isZeroSize` or not? So 
lldb doesn't have to put any particular value here?

https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Revert "Add option to generate additional debug info for expression dereferencing pointer to pointers. #94100" (PR #95174)

[David Blaikie via cfe-commits]

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

[clang] Bump the DWARF version number to 5 on Darwin. (PR #95164)

[David Blaikie via 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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Add option to generate additional debug info for expression dereferencing pointer to pointers. (PR #94100)

[David Blaikie via cfe-commits]

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 generating additional loads and 
> stores with debug-info enabled, which is usually highly undesirable. 

I hope that isn't the intent, I thought the intent was just to add more debug 
info for loads and stores to carry more intermediate type information.

Sorry I missed that this patch does change clangs generated ir. Seriously 
overlooked on my part.

>  * Instead of creating an alloca,  store+load, and dbg.declare, connect a 
> dbg.value to the relevant pointer Value. 
> 
> The 2nd point is more work, but it means you're only adding more debug-info, 
> and nothing else. A dbg.value connected to the pointer Value is the 
> inevitable consequence of the alloca+store+load idiom after mem2reg/SROA 
> anyway.

Yeah, pretty sure (2) would be the preferred direction.


https://github.com/llvm/llvm-project/pull/94100
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -1830,6 +1830,9 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables 
&CGVT,
   if (VTable->hasInitializer())
 return;
 
+  if (RD->shouldEmitInExternalSource())
+return;

dwblaikie wrote:

But this logic (using available externally definitions) is already built into 
the modules-codegen work (though it's probably only for functions, I guess it 
might not be in a place general enough to apply to variables as well - if it 
either can't be generalized in that way, or even if it just isn't suffiicently 
general now, I agree that it'd be a separate patch, but probably one worth 
doing) - again, if/where possible, it'd be nice if these things 
aligned/involved fewer special cases and inconsistencies.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -1830,6 +1830,9 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables 
&CGVT,
   if (VTable->hasInitializer())
 return;
 
+  if (RD->shouldEmitInExternalSource())
+return;

dwblaikie wrote:

Yes, and we do emit them in normal builds above -O0 if the vtable is homed 
elsewhere due to a key function for instance - they can be used to allow for 
devirtualization

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -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 vtable there.
+  if (const auto *Class = dyn_cast(D))
+return Class->isInCurrentModuleUnit() &&
+   Class->getDefinition() && Class->isDynamicClass();
+

dwblaikie wrote:

Why is this needed? Why doesn't it fall out of the condition above that covers 
`hasExternalDefinitions`?

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -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 class is templated, the tables are emitted in every object that
+  // references any of them.
+  // - Otherwise, if the class is attached to a module, the tables are uniquely
+  // emitted in the object for the module unit in which it is defined.
+  // - Otherwise, if the class has a key function (see below), the tables are
+  // emitted in the object for the translation unit containing the definition 
of
+  // the key function. This is unique if the key function is not inline.
+  // - Otherwise, the tables are emitted in every object that references any of
+  // them.
+  if (RD->isInNamedModule())
+return RD->shouldEmitInExternalSource();

dwblaikie wrote:

Shouldn't need to test "isInNamedModule" here, though? Would it be adequate to 
do:
```
if (RD->shouldEmitInExternalSource())
  return true;
```

Ah, I geuss you want to return false ASAP too - I guess looking at some of the 
implementation details of `shouldEmitInExternalSource` and using those APIs 
directly here - Always -> true, Never -> false, I think?

Like `adjustGVALinkageForExternalDefinitionKind` already does? Or even reusing 
some of that functionality somehow.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

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 list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -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 class is templated, the tables are emitted in every object that
+  // references any of them.
+  // - Otherwise, if the class is attached to a module, the tables are uniquely
+  // emitted in the object for the module unit in which it is defined.
+  // - Otherwise, if the class has a key function (see below), the tables are
+  // emitted in the object for the translation unit containing the definition 
of
+  // the key function. This is unique if the key function is not inline.
+  // - Otherwise, the tables are emitted in every object that references any of
+  // them.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())

dwblaikie wrote:

The plumbing's already there though - and means one source of truth that both 
the consumer of the .pcm and the compilation of the .pcm->.o step use - rather 
than two separate pieces of code that also have to be kept in sync. (and the 
code's already there, the check before the one this patch proposes to be added 
to `isConsumerInterestedIn` is the modular code generation codepath already - 
and the populating of `ModularCodegenDecls` is part of that system)

I'd rather not add ad-hoc hardcoded things across these several places when we 
have a mechanism to use that gets set at .pcm time, is (sort of) generic across 
that for the usage and the implementation compilation.

It seems like fine existing infrastructure to further generalize to handle 
home-able types (that can be homed for code generation reasons, as well as for 
debug info reasons).

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] Add option to generate additional debug info for expression dereferencing pointer to pointers. (PR #94100)

[David Blaikie via cfe-commits]

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 hashes, and a little detail on the 
updates to address the reason for the revert (adding the explicit triple to the 
test).

https://github.com/llvm/llvm-project/pull/94100
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 6e975ec - Reapply "[DebugInfo] Add flag to only emit referenced member functions" (#93767)

[David Blaikie via cfe-commits]

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

LOG: Reapply "[DebugInfo] Add flag to only emit referenced member functions" 
(#93767)

This reverts commit 02c6845c762dfd0a19d4a2f997990e160f392dae,
reapplying bfabc958c7c0d7ddc15f23383d9da836e8c6093f.

The patch was reverted due to the test failing on MacOS and Windows
where type units aren't supported. This is addressed by limiting type
unit flag/test coverage to Linux.

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
anyway). So add a flag to opt out of this behavior for experimenting
with this more terse behavior.

I think Sony already does this by default, so perhaps with a change to
the defaults, Sony can migrate to this rather than a downstream patch.

This breaks current debuggers in some expected ways - but those
breakages are visible without this feature too. Consider member function
template instantiations - they can't be consistently enumerated in every
translation unit:

a.h:
```
struct t1 {
  template 
  static int f1() {
return i;
  }
};
namespace ns {
template 
int f1() {
  return i;
}
}  // namespace ns
```
a.cpp:
```
void f1() {
  t1::f1<0>();
  ns::f1<0>();
}
```
b.cpp:
```
void f1();
int main() {
  f1();
  t1::f1<1>();
  ns::f1<1>();
}
```
```
(gdb) p ns::f1<0>()
$1 = 0
(gdb) p ns::f1<1>()
$2 = 1
(gdb) p t1::f1<0>()
Couldn't find method t1::f1<0>
(gdb) p t1::f1<1>()
$3 = 1
(gdb) s
f1 () at a.cpp:3
3 t1::f1<0>();
(gdb) p t1::f1<0>()
$4 = 0
(gdb) p t1::f1<1>()
Couldn't find method t1::f1<1>
(gdb)
```

(other similar non-canonical features are implicit special members
(copy/move ctor/assignment operator, default ctor) and nested types (eg:
pimpl idiom, where the nested type is declared-but-not-defined in one
TU, and defined in another TU))

lldb can't parse the template expressions above, so I'm not sure how to
test it there, but I'd guess it has similar problems. (

https://stackoverflow.com/questions/64602475/how-to-print-value-returned-by-template-member-function-in-gdb-lldb-debugging
so... I guess that's just totally not supported in lldb, how
unfortunate. And implicit special members are instantiated implicitly by
lldb, so missing those doesn't tickle the same issue)

Some very rudimentary numbers for a clang debug build:
.debug_info section size:
-g: 476MiB
-g -fdebug-types-section: 357MiB
-g -gomit-unreferenced-members: 340MiB

Though it also means a major reduction in .debug_str size,
-fdebug-types-section doesn't reduce string usage (so the first two
examples have the same .debug_str size, 247MiB), down to 175MiB.

So for total clang binary size (I don't have a quick "debug section size
reduction" on-hand): 1.45 (no type units) GiB -> 1.34 -> 1.22, so it
saves about 120MiB of binary size.

Original Differential Revision: https://reviews.llvm.org/D152017

Added: 
clang/test/CodeGenCXX/debug-info-incomplete-types.cpp

Modified: 
clang/include/clang/Basic/DebugOptions.def
clang/include/clang/Driver/Options.td
clang/lib/CodeGen/CGDebugInfo.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/debug-options.c

Removed: 




diff  --git a/clang/include/clang/Basic/DebugOptions.def 
b/clang/include/clang/Basic/DebugOptions.def
index b94f6aef9ac60b..bc96d5dfdf890b 100644
--- a/clang/include/clang/Basic/DebugOptions.def
+++ b/clang/include/clang/Basic/DebugOptions.def
@@ -68,6 +68,8 @@ BENIGN_DEBUGOPT(NoInlineLineTables, 1, 0) ///< Whether debug 
info should contain
   ///< inline line tables.
 
 DEBUGOPT(DebugStrictDwarf, 1, 1) ///< Whether or not to use strict DWARF info.
+DEBUGOPT(DebugOmitUnreferencedMethods, 1, 0) ///< Omit unreferenced member
+///< functions in type debug info.
 
 /// Control the Assignment Tracking debug info feature.
 BENIGN_ENUM_DEBUGOPT(AssignmentTrackingMode, AssignmentTrackingOpts, 2,

diff  --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 1637a114fcce15..57f37c5023110f 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4345,6 +4345,10 @@ defm strict_dwarf : BoolOption<"g", "strict-dwarf",
   "the specified version, avoiding features from later versions.">,
   NegFlag, BothFlags<[], [ClangOption, CLOption, DXCOption]>>,
   Group;
+defm omit_unreferenced_methods : BoolGOption<"omit-unreferenced-methods",
+  CodeGenOpts<"DebugOmitUnreferencedMethods">, DefaultFalse,
+  NegFlag,
+  PosFlag, BothFlags<[], [ClangOptio

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -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 class is templated, the tables are emitted in every object that
+  // references any of them.
+  // - Otherwise, if the class is attached to a module, the tables are uniquely
+  // emitted in the object for the module unit in which it is defined.
+  // - Otherwise, if the class has a key function (see below), the tables are
+  // emitted in the object for the translation unit containing the definition 
of
+  // the key function. This is unique if the key function is not inline.
+  // - Otherwise, the tables are emitted in every object that references any of
+  // them.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())

dwblaikie wrote:

`hasExternalDefinitions` isn't "is this AST from some other AST file" but "did 
the AST file say it'd handle the object-file definition of this thing" - the 
pcm file specifies which things it'll handle.

So if the intent is not to home anything in the GMF, then pcms could be written 
out that don't set "hasExternalDefinitions" to true for those entities.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -1802,6 +1802,12 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables 
&CGVT,
   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 (Module *M = RD->getOwningModule();
+  M && M->isNamedModule() && M != CGM.getContext().getCurrentNamedModule())

dwblaikie wrote:

Generally we do such cleanup before if there's already at least one use of the 
idiom that could be refactored without the patch applied, or in the patch - 
rather than after. (sometimes there's a need to do these things after, but this 
seems like a fine thing to include in this patch, or 

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [lldb] [clang][lldb] Don't assert structure layout correctness for layouts provided by LLDB (PR #93809)

[David Blaikie via cfe-commits]

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 debugger or the compiler frontend... but I'll certainly leave it up to 
@AaronBallman as to where he thinks the right tradeoff/sweetspot is.

https://github.com/llvm/llvm-project/pull/93809
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Revert "[DebugInfo] Add flag to only emit referenced member functions" (PR #93767)

[David Blaikie via cfe-commits]

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

[clang] [llvm] Add option to generate additional debug info for expression dereferencing pointer to pointers. (PR #81545)

[David Blaikie via 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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] ea1ecb5 - Fix test - remove unnecessary/incorrect `-S`, in favor of `-emit-llvm`

[David Blaikie via cfe-commits]

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

LOG: Fix test - remove unnecessary/incorrect `-S`, in favor of `-emit-llvm`

Added: 


Modified: 
clang/test/CodeGenCXX/debug-info-ptr-to-ptr.cpp

Removed: 




diff  --git a/clang/test/CodeGenCXX/debug-info-ptr-to-ptr.cpp 
b/clang/test/CodeGenCXX/debug-info-ptr-to-ptr.cpp
index 8e465a1febf7c..0885e7076d51c 100644
--- a/clang/test/CodeGenCXX/debug-info-ptr-to-ptr.cpp
+++ b/clang/test/CodeGenCXX/debug-info-ptr-to-ptr.cpp
@@ -1,6 +1,6 @@
 // Test debug info for intermediate value of a chained pointer deferencing
 // expression when the flag -fdebug-info-for-pointer-type is enabled.
-// RUN: %clang_cc1 %s -fdebug-info-for-profiling -debug-info-kind=constructor 
-S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -fdebug-info-for-profiling -debug-info-kind=constructor 
-emit-llvm -o - | FileCheck %s
 
 class A {
 public:



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via cfe-commits]

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

[clang] [llvm] Add option to generate additional debug info for expression dereferencing pointer to pointers. (PR #81545)

[David Blaikie via 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://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [BPF] Fix linking issues in static map initializers (PR #91310)

[David Blaikie via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF (PR #91423)

[David Blaikie via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -1802,6 +1802,12 @@ void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables 
&CGVT,
   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 (Module *M = RD->getOwningModule();
+  M && M->isNamedModule() && M != CGM.getContext().getCurrentNamedModule())

dwblaikie wrote:

this idiom is repeated in a few places, and is a bit long-winded. Perhaps it 
could be made into some common function/shared (if the `hasExternalDefinition` 
thing can't be made to work more generally)

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -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 class is templated, the tables are emitted in every object that
+  // references any of them.
+  // - Otherwise, if the class is attached to a module, the tables are uniquely
+  // emitted in the object for the module unit in which it is defined.
+  // - Otherwise, if the class has a key function (see below), the tables are
+  // emitted in the object for the translation unit containing the definition 
of
+  // the key function. This is unique if the key function is not inline.
+  // - Otherwise, the tables are emitted in every object that references any of
+  // them.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())

dwblaikie wrote:

Rather than doing this based on a named module, I wonder if this could be 
powered by the same system that originally worked for clang header modular code 
generation. It worked/works by adding a bit to declarations about whether to 
home them to a modular object file

e6b7c28d17edf428c58ba65c549109c0a488a5be

Though I guess in 1ac9c98e6c40d9a1e6cb904b414161388af4a89c I used the bit on a 
type to mean "home this type in debug info" and the bit on a function to mean 
"home this function's code" - so I'm not sure what we'd do to track the vtable 
specifically... Maybe not worth it/not feasible, not sure.


https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [C++20] [Modules] [Itanium ABI] Generate the vtable in the module unit of dynamic classes (PR #75912)

[David Blaikie via cfe-commits]

@@ -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 class is templated, the tables are emitted in every object that
+  // references any of them.
+  // - Otherwise, if the class is attached to a module, the tables are uniquely
+  // emitted in the object for the module unit in which it is defined.
+  // - Otherwise, if the class has a key function (see below), the tables are
+  // emitted in the object for the translation unit containing the definition 
of
+  // the key function. This is unique if the key function is not inline.
+  // - Otherwise, the tables are emitted in every object that references any of
+  // them.
+  if (Module *M = RD->getOwningModule(); M && M->isNamedModule())

dwblaikie wrote:

I guess doing it through the `hasExternalDefinitions` system suggested above 
might reduce the number of special cases here, if there was a nice way to do 
address the code V debug info use.

https://github.com/llvm/llvm-project/pull/75912
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[David Blaikie via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Serialization] Check for stack exhaustion when reading declarations (PR #79875)

[David Blaikie via cfe-commits]

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?

https://github.com/llvm/llvm-project/pull/79875
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [Clang] Fix definition of layout-compatible to ignore empty classes (PR #92103)

[David Blaikie via cfe-commits]

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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [Clang] Fix definition of layout-compatible to ignore empty classes (PR #92103)

[David Blaikie via cfe-commits]

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 Playstation, and Apple's platforms) that are only interested in 
Clang ABI compatibility can retain the old behavior.

@rjmccall might have an interest in whether this is an ABI thing Apple cares 
about
@pogo59 not sure who over at Sony's the best person to check on sony ABI things

https://github.com/llvm/llvm-project/pull/92103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [llvm] Add triples for managarm (PR #87845)

[David Blaikie via cfe-commits]

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?

https://github.com/llvm/llvm-project/pull/87845
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via cfe-commits]

@@ -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", "omit-unreferenced-members",
+  CodeGenOpts<"DebugOmitUnreferencedMembers">, DefaultFalse,
+  PosFlag,
+  NegFlag, BothFlags<[], [ClangOption, CLOption, DXCOption]>>,
+  Group;

dwblaikie wrote:

I looked into this - and I couldn't find any example where MarshallingInfoFlag 
would allow us to omit the renderDebugOptions code - do you have an example in 
mind? 

And it seems like BoolOption could even be replaced with BoolGOption, and that 
either of those does use Marshalling stuff? (the BoolOption< def in clang 
Options.td has some `MarshalledFlagRec` stuff at the end?)

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

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via cfe-commits]

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". But 
anyway, since you're likeyl the first folks to use this, I've made that change 
toward "methods".

> I'm +1 on having this be non-default; adding it to SCE tuning is also not 
> necessary (or desired) for now, because this is more aggressive than our 
> private option (we keep entries for member functions that are called), and 
> we're still working out whether we can/should adopt this instead.

Yeah, no plans to make this the default any time soon - will start with it 
entirely off by default, and figured you Sony folks can see if it can be your 
default in the future - save you carrying the downstream patches, gives you 
some hope other folks might adopt the same strategy (& folks considering 
adopting it would have some reassurance that other people are living with 
it/interested in caring for it already)


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

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via cfe-commits]

@@ -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", "omit-unreferenced-members",
+  CodeGenOpts<"DebugOmitUnreferencedMembers">, DefaultFalse,
+  PosFlag,
+  NegFlag, BothFlags<[], [ClangOption, CLOption, DXCOption]>>,
+  Group;

dwblaikie wrote:

Wasn't able to get this to avoid the `renderDebugOptions` code - even other 
uses of marshalling (and it looks like BoolOption, and even BoolGOption, have 
some marshalling details in their implementation - so maybe they're getting the 
same functionality as MarshallingInfoFlag already?) seem to still have to 
handle/repeat the flag from the driver to the frontend.

Oh, and now that I'm checking compatible flags in the driver (disabling this 
feature if -fstandalone-debug or -fdebug-types-section are enabled) then 
there's probably no avoiding having some code there anyway.

Switching to BoolGOption does make it a bit tidier, though.

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

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via cfe-commits]

@@ -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 && !CGM.getCodeGenOpts().DebugOmitUnreferencedMembers)

dwblaikie wrote:

Added that at the driver level - and similarly for type units (since they use 
an mllvm flag, clang frontend/codegen doesn't actually know if type units are 
enabled). (though this doesn't work out /that/ badly for type units, in some 
sense - the type units are still consistent, they just consistently contain no 
member functions (in the same way that even without the flag, for things like 
member function template instantiations we never put them in the member list, 
so they never end up in type units - only attached as declarations to the 
skeleton type DIE in the CU))

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

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via cfe-commits]

@@ -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 mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [DebugInfo] Add flag to only emit referenced member functions (PR #87018)

[David Blaikie via 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
 functions

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
anyway). So add a flag to opt out of this behavior for experimenting
with this more terse behavior.

I think Sony already does this by default, so perhaps with a change to
the defaults, Sony can migrate to this rather than a downstream patch.

This breaks current debuggers in some expected ways - but those
breakages are visible without this feature too. Consider member function
template instantiations - they can't be consistently enumerated in every
translation unit:

a.h:
```
struct t1 {
  template 
  static int f1() {
return i;
  }
};
namespace ns {
template 
int f1() {
  return i;
}
}  // namespace ns
```
a.cpp:
```
void f1() {
  t1::f1<0>();
  ns::f1<0>();
}
```
b.cpp:
```
void f1();
int main() {
  f1();
  t1::f1<1>();
  ns::f1<1>();
}
```
```
(gdb) p ns::f1<0>()
$1 = 0
(gdb) p ns::f1<1>()
$2 = 1
(gdb) p t1::f1<0>()
Couldn't find method t1::f1<0>
(gdb) p t1::f1<1>()
$3 = 1
(gdb) s
f1 () at a.cpp:3
3 t1::f1<0>();
(gdb) p t1::f1<0>()
$4 = 0
(gdb) p t1::f1<1>()
Couldn't find method t1::f1<1>
(gdb)
```

(other similar non-canonical features are implicit special members
(copy/move ctor/assignment operator, default ctor) and nested types (eg:
pimpl idiom, where the nested type is declared-but-not-defined in one
TU, and defined in another TU))

lldb can't parse the template expressions above, so I'm not sure how to
test it there, but I'd guess it has similar problems. (
https://stackoverflow.com/questions/64602475/how-to-print-value-returned-by-template-member-function-in-gdb-lldb-debugging
so... I guess that's just totally not supported in lldb, how
unfortunate. And implicit special members are instantiated implicitly by
lldb, so missing those doesn't tickle the same issue)

Some very rudimentary numbers for a clang debug build:
.debug_info section size:
-g: 476MiB
-g -fdebug-types-section: 357MiB
-g -gomit-unreferenced-members: 340MiB

Though it also means a major reduction in .debug_str size,
-fdebug-types-section doesn't reduce string usage (so the first two
examples have the same .debug_str size, 247MiB), down to 175MiB.

So for total clang binary size (I don't have a quick "debug section size
reduction" on-hand): 1.45 (no type units) GiB -> 1.34 -> 1.22, so it
saves about 120MiB of binary size.

Also open to any riffing on the flag name for sure.

@probinson - would this be an accurate upstreaming of your internal 
handling/would you use this functionality? If it wouldn't be useful to you, 
it's maybe not worth adding upstream yet - not sure we'll use it at Google, but 
if it was useful to you folks and meant other folks could test with it it 
seemed maybe useful.

Original Differential Revision: https://reviews.llvm.org/D152017
---
 clang/include/clang/Basic/DebugOptions.def   |  2 ++
 clang/include/clang/Driver/Options.td|  7 +++
 clang/lib/CodeGen/CGDebugInfo.cpp|  2 +-
 clang/lib/Driver/ToolChains/Clang.cpp|  6 ++
 .../test/CodeGenCXX/debug-info-incomplete-types.cpp  | 12 
 clang/test/Driver/debug-options.c|  6 ++
 6 files changed, 34 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/CodeGenCXX/debug-info-incomplete-types.cpp

diff --git a/clang/include/clang/Basic/DebugOptions.def 
b/clang/include/clang/Basic/DebugOptions.def
index 7cd3edf08a17e..fe2adaa20f4e3 100644
--- a/clang/include/clang/Basic/DebugOptions.def
+++ b/clang/include/clang/Basic/DebugOptions.def
@@ -68,6 +68,8 @@ BENIGN_DEBUGOPT(NoInlineLineTables, 1, 0) ///< Whether debug 
info should contain
   ///< inline line tables.
 
 DEBUGOPT(DebugStrictDwarf, 1, 1) ///< Whether or not to use strict DWARF info.
+DEBUGOPT(DebugOmitUnreferencedMembers, 1, 0) ///< Omit unreferenced member
+///< functions in type debug info.
 
 /// Control the Assignment Tracking debug info feature.
 BENIGN_ENUM_DEBUGOPT(AssignmentTrackingMode, AssignmentTrackingOpts, 2,
diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 29066ea14280c..4000403a1991d 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -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

[clang] [llvm] [Clang] Fix definition of layout-compatible to ignore empty classes (PR #92103)

[David Blaikie via cfe-commits]

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?

https://github.com/llvm/llvm-project/pull/92103
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

@@ -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 fussed about the separator for the prefix (we control that) and 
category (will the category be user-visible? Like a warning group? Or is that 
only still an implementation detail/contract between DWARF producer and DWARF 
consumer? I thought it was more the latter/implementation detail? In which case 
we can say what characters are valid there, and I'd keep it pretty simple, like 
the warning group names - all lower, dash or underscore separated seems fine)

https://github.com/llvm/llvm-project/pull/79230
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Add support for builtin_verbose_trap (PR #79230)

[David Blaikie via cfe-commits]

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... (sorry, I think I saw this was discussed 
somewhere on the review, but I can't find where now... :/)

I guess it came up here: 
https://github.com/llvm/llvm-project/pull/79230#discussion_r1591335258 and at 
the time it still had `llvm` in it, wasn't clear conversation that showed how 
the alternative conclusion was reached.

https://github.com/llvm/llvm-project/pull/79230
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [flang] [llvm] [mlir] [polly] [test]: fix filecheck annotation typos (PR #91854)

[David Blaikie via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [MC,llvm-readobj,yaml2obj] Support CREL relocation format (PR #91280)

[David Blaikie via cfe-commits]

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 so long as you're comfy breaking it in the 
future (& it's not like the current allocated numbers are anywhere near... so 
by the time anyone is using this allocated number for an official purposes, 
this experiment will be long gone & hopefully have an official number) until it 
gets an official number, I guess that works for me. Thanks!

https://github.com/llvm/llvm-project/pull/91280
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [MC,clang,llvm-readobj,yaml2obj] Support CREL relocation format (PR #91280)

[David Blaikie via cfe-commits]

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 than "enable" (or no verb at all), as in 
`-allow-experimental-crel`? 

> While ELF allows extension space for processor/OS/application-specific 
> section types, using them introduces challenges
since we want linker errors when SHT_CREL is unsupported (see 
isKnownSpecificSectionType in https://github.com/llvm/llvm-project/pull/85173):
> 
> [SHT_LOUSER,SHT_HIUSER]: Not applicable as static relocation sections do not 
> have the SHF_ALLOC flag.

I missed a step here - why is this range only applicable for SHF_ALLOC 
sections? Ah, from the patch you linked earlier, linkers don't fail on 
SHT_*USER range unless it's allocated... that seems a bit tenuous, though.

Seems like this the range intended for this kind of experimentation, with the 
cost that it doesn't produce an error from existing linkers (though it could, 
but they're trying to be conservative/allow for extensions that don't need 
linker-awareness).

Guess I'll leave that up to other folks to comment on.

> [SHT_LOOS,SHT_HIOS]: Requires annoying SHF_OS_NONCONFORMING. We want psABI 
> documents to be willing to accept CREL when toolchain becomes ready, 
> especially if their relocatable files are huge due to linker relaxation (e.g. 
> RISC-V, LoongArch), we want to avoid GNUism / LLVMism.

So in this case each OS would have to allocate their own number in in the 
SHT_*OS range? Yeah, that seems unfortunate.

> [SHT_LOPROC,SHT_HIPROC]: Processor-specific and not applicable.

Similar to the *OS case above, I guess.

> We will unlikely change the format. 

I think we should start with the theory that the format will change - even if 
it doesn't, we wouldn't want to introduce this new thing and avoid change 
because we've made it hard to change if we discover in early testing that 
there's things we could do better.

> Solaris-specific SHF_ORDERED and SHF_EXCLUDE, without a SunOS/Solaris name 
> infix/prefix/suffix, make them into glibc elf.h.
> SHF_EXCLUDE gets picked due to its linker semantics but SHF_ORDERED is never 
> used.
> A generic name SHT_CREL might not be fundamentally different from SHF_EXCLUDE.

Got links to the history here, about how SHF_ORDERED/EXCLUDE came to be?

https://github.com/llvm/llvm-project/pull/91280
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [MC,clang,llvm-readobj,yaml2obj] Support CREL relocation format (PR #91280)

[David Blaikie via cfe-commits]

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 equally, it does seem suitable (to try to 
use a name less likely to collide), if this is an experiment ,but I understand 
other projects may have some not-invented-here negative sentiment to such a 
thing), perhaps "SHT_EXP" wouldn't be too bad?

https://github.com/llvm/llvm-project/pull/91280
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [MC,clang,llvm-readobj,yaml2obj] Support CREL relocation format (PR #91280)

[David Blaikie via cfe-commits]

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 hesitate to comment on the specifics while it's all together as I imagine 
it'll become hard to follow the different commentary across the larger 
patch/different project areas, etc.

(though the flag design seems strange to me - having --crel and 
--experimental-crel, rather than having only the latter (and renaming it in the 
future, rather than using two flags now and removing one in the future))



https://github.com/llvm/llvm-project/pull/91280
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [Modules] Detect ODR mismatches for enums in non-C++ like in C++. (PR #90298)

[David Blaikie via cfe-commits]

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 different modules.

But it sounds like in C you're suggesting that we (moreso with this patch) 
detect mismatches and silently allow different type definitions to work? Which 
would presumably be the right thing for C.

So, I guess, maybe my concern is only with a confusing title/description... 

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