[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Fair enough - all seems a bit unfortunate to be pushing these attributes 
through to places they don't technically apply to (both complicates the 
implementation, and might be confusing to users).

Thanks for trying the prototype - not clear what the right design is, so let's 
go with what you've got here.




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1249
+  llvm::DIType *WrappedDI = getOrCreateType(WrappedTy, Unit);
+  if (Annotations.size() == 0)
+return WrappedDI;

use `empty()` rather than `size() == 0`



Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1252
+
+  if (WrappedDI == nullptr)
+WrappedDI = DBuilder.createUnspecifiedType("void");

probably write this as `if (!WrappedDI)`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

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

[PATCH] D159483: [Modules] Add a flag to control builtin headers being in system modules

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: sammccall, dblaikie.
dblaikie added a comment.

@sammccall @rsmith - figure if this does impact us, we'll use the flag to 
opt-out in the short term while we figure out longer-term solution, yeah? Is 
there any pre-emptive testing you think is worth doing?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159483/new/

https://reviews.llvm.org/D159483

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

[PATCH] D158296: [clang] Diagnose overly complex Record in __builtin_dump_struct

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@rsmith ping on this thread - wondering what you'd prefer the direction be here


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158296/new/

https://reviews.llvm.org/D158296

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D150226#4650244 , @carlosgalvezp 
wrote:

> @shafik @aaron.ballman @dblaikie
>
> Hello! Just wanted to check if there's any blockers for merging this patch? 
> We are now on Clang 18, i.e. 2 releases after the warning was introduced, so 
> IMO I believe it's a good time to turn it into a hard error and test it in 
> the wild.
>
> I read concerns about breaking code. Technically, UB is only invoked in C++17 
> and onwards (before, it's only unspecified behavior) - could a solution to 
> mitigate risk/break less code be to only enable this hard error in C++17? 
> This way, only people who use a relatively new C++ Standard and compiler 
> would get the error.
>
> I also wonder what is the way forward with respect to code reviews, since 
> Phabricator is deprecated. @shafik do you intend to continue here, or will 
> you move this into a Github PR?
>
> Happy to help if there's anything I can do! Thanks for the great work :)

I still think even if we can subset this, for whatever we're going to turn into 
a hard error, it should be a warning-as-error in system headers first for at 
least a release. (so perhaps the transition should look like: null (no 
diagnostic) -> warning -> warning-default-to-error -> 
warning-default-to-error-even-in-system-headers -> hard error)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

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

[PATCH] D158730: [DebugMetadata][DwarfDebug] Don't retain local types with -fno-eliminate-unused-debug-types

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

Nice and easy


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158730/new/

https://reviews.llvm.org/D158730

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

[PATCH] D157615: [ExtendLifetimes][1/4] Add "disable-post-ra" function attribute to disable the post-regalloc scheduler

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D157615#4647235 , @StephenTozer 
wrote:

> In D157615#4646848 , @dblaikie 
> wrote:
>
>> Might be worth rewording the commit, or  splitting it - I'd say the 
>> introduction of `optdebug` should be the noteworthier part of this patch (or 
>> whichever patch introduces it) - so either "this patch adds optdebug, and a 
>> first/exemplar use of it in postra scheduler" or split it into a patch that 
>> just adds the attribute and no uses, and one that adds the use.
>
> SGTM, the scope of this patch has increased as-of the latest update. That 
> being said AFAIK Github PRs don't currently have support for stacked PRs, so 
> I imagine I'll need to have one review up at a time.

Yeah, don't think we have a clear path for stacked PRs right now - so, yeah, 
will try to work through them one at a time. I guess you could post PRs that 
contain all the commits (eg: one review containing commit A, another review 
containing A and B, etc) - it doesn't have a good solution if large changes to 
predecessor patches need updates to latter patches (eg: if A causes invasive 
changes to B) - but otherwise it's probably OK-ish... (& in this case I don't 
think any changes to "adding optdebug" are likely to substantially change the 
nature of subsequent patches). But maybe not a big deal either way/can just 
wait for the subsequent patches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157615/new/

https://reviews.llvm.org/D157615

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

[PATCH] D157615: [ExtendLifetimes][1/4] Add "disable-post-ra" function attribute to disable the post-regalloc scheduler

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Might be worth rewording the commit, or  splitting it - I'd say the 
introduction of `optdebug` should be the noteworthier part of this patch (or 
whichever patch introduces it) - so either "this patch adds optdebug, and a 
first/exemplar use of it in postra scheduler" or split it into a patch that 
just adds the attribute and no uses, and one that adds the use. (I'd lean 
towards splitting it up, personally & I guess maybe moving to github pull 
requests in the process, perhaps)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157615/new/

https://reviews.llvm.org/D157615

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

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14524
+  VD->hasAttr() &&
+  (!VD->getType().isPODType(getASTContext()) ||
+   !VD->getType().isConstQualified())) {

aeubanks wrote:
> rnk wrote:
> > I don't think `isPODType` really describes what we want. I think we want 
> > `isConstantStorage` which @dblaikie just added, which means essentially, 
> > can this global be placed in a readonly section.
> @dblaikie is 
> [this](https://github.com/llvm/llvm-project/blob/08d7377b67358496a409080fac22f3f7c077fb63/clang/lib/AST/Type.cpp#L124)
>  missing a check for a trivial constructor? using `isConstantStorage` the 
> test is failing on `const Foo` by warning there when it shouldn't be
(I realize this patch has been abandoned for now (probably worth actually 
marking it abandoned - I'm guessing folks are going to be wanting to look at 
outstanding phab reviews to try to get them cleaned out as we transition to 
github pull requests), just leaving some comments for the sake of it)

I think the `isConstantStorage` made some assumptions that the construction 
issues were already handled externally to the call... or you could pass in a 
bool to the second argument `ExcludeCtor` - but it's overly coarse/simplistic 
and I think just says "if you're not excluding the ctor, and the type is a 
CXXRecordDecl, then it's non-trivial" (so you'd pass in `false` for 
`ExcludeCtor` if you know the ctor you're using is non-trivial, otherwise pass 
in `true`). Because the function doesn't know which ctor you're using, so it's 
up to the caller to check. The dtor is checked, however (if `ExcludeDtor` is 
false).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158223/new/

https://reviews.llvm.org/D158223

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

[PATCH] D158296: [clang] Diagnose overly complex Record in __builtin_dump_struct

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:732-733
+int RDKind = RD->isClass() ? 0 : (RD->isStruct() ? 1 : 2);
+S.Diag(PtrArg->getBeginLoc(), diag::err_builtin_dump_struct_too_complex)
+<< RDKind << RD->getName();
+return ExprError();

yronglin wrote:
> rsmith wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > aaron.ballman wrote:
> > > > > This will correctly handle diagnosing a gigantic anonymous struct.
> > > > Producing an error here seems likely to eventually cause problems in 
> > > > practice for some users: people are using `__builtin_dump_struct` in 
> > > > generic code for reflection purposes, not just for debugging, and this 
> > > > will cause us to start rejecting complex generic code.
> > > > 
> > > > Instead of rejecting, can we produce a tree of `PseudoObjectExpr`s if 
> > > > we have too many steps to store in a single expression?
> > > > Producing an error here seems likely to eventually cause problems in 
> > > > practice for some users: people are using __builtin_dump_struct in 
> > > > generic code for reflection purposes, not just for debugging, and this 
> > > > will cause us to start rejecting complex generic code.
> > > >
> > > > Instead of rejecting, can we produce a tree of PseudoObjectExprs if we 
> > > > have too many steps to store in a single expression?
> > > 
> > > I think that requires wider discussion -- I don't think 
> > > `__builtin_dump_struct` is a reasonable interface we want to support for 
> > > reflection (in fact, I'd argue it's an explicit non-goal, the same as 
> > > reflection via `-ast-dump`). Compile-time reflection is something we're 
> > > likely to need to support more intentionally and I don't think we're 
> > > going to want to use this as an interface for it or have to maintain it 
> > > as a reflection tool long-term. As such, I think producing a tree of 
> > > `PseudoObjectExpr`s is overkill; you can quote me on this a few years 
> > > from now when we're laughing at its quaintness, but "16k fields of debug 
> > > output is enough for anyone" for a debugging interface.
> > > 
> > > (That said, I think we should be considering what support we want to add 
> > > to the compiler for reflection in service of the work being done in WG21 
> > > on the topic -- if `__builtin_dump_struct` is being used for reflection 
> > > in practice, it would be nice to give people a supported, more ergonomic 
> > > interface for it that we can use for a future version of C++.)
> > The bug report https://github.com/llvm/llvm-project/issues/63169 was 
> > encountered by a user hitting the previous 256-element limit in practice 
> > when using `__builtin_dump_struct` for reflection. I don't think we can 
> > reasonably prevent that from happening, other than -- as you say -- 
> > encouraging WG21 to give us a real reflection design we can implement.
> fixed.
@rsmith what do you think we should do now? Support even larger values than 
uint16_t worth? (the tree `PseudoObjectExpr` suggestion you made?)

If we did make the uint16_t limit stand - perhaps that'd apply pressure to 
folks who have been using this for RTTI to consider other options - it wouldn't 
break any existing code (that's not already broken in clang, at least - with 
the overflow stuff happening) but might put pressure on further growth of such 
techniques?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158296/new/

https://reviews.llvm.org/D158296

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

[PATCH] D158496: [WIP][clang][modules] module build daemon initial commit

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

(probably worth linking to the RFC/etc 
(https://discourse.llvm.org/t/rfc-modules-build-daemon-build-system-agnostic-support-for-explicitly-built-modules/71524/47)
 from the patch description/commit message)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158496/new/

https://reviews.llvm.org/D158496

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

[PATCH] D151575: [clang][diagnostics] Always show include stacks on top-level diagnostics

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> Maybe we should consider adding a flag to set diagnostic verbosity level? 
> e.g., -fdiagnostic-verbosity=terse|default|verbose where terse never prints 
> notes or include stacks, default is what we do today, and verbose always 
> prints include stacks?

My guess would be that not enough people would discover and use it, so we 
shouldn't do that.

I guess a basic question: what does GCC do about these "included from" stacks 
compared to clang?

Also it seemed like on the bug there was a more narrower issue being discussed 
- that the notes were missing from a second error because it had the same 
include stack as a note attached to a previous error, but not the same include 
stack as the previous error (which is the last thing with the include stack 
mentioned)? I may've missed the point where it generalized from that particular 
bug to "let's put the include stack always instead" rather than addressing that 
mismatch of looking at the last message in general, rather than the last 
non-note message (I guess the include stack is only printed for non-notes, by 
the looks of the bug example? So those are the ones that should be 
cached/checked against)

But equally, I suspect it's probably OK to do it unconditionally... probably.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151575/new/

https://reviews.llvm.org/D151575

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

aaron.ballman wrote:
> yronglin wrote:
> > aaron.ballman wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > yronglin wrote:
> > > > > > yronglin wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > yronglin wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > yronglin wrote:
> > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > > > > > Could/should we add some error checking in the 
> > > > > > > > > > > > > > > > ctor to assert that we don't overflow these 
> > > > > > > > > > > > > > > > longer values/just hit the bug later on?
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > > (& could we use `unsigned short` here rather 
> > > > > > > > > > > > > > > > than bitfields?)
> > > > > > > > > > > > > > > We've already got them packed in with other 
> > > > > > > > > > > > > > > bit-fields from the expression bits, so I think 
> > > > > > > > > > > > > > > it's reasonable to continue the pattern of using 
> > > > > > > > > > > > > > > bit-fields (that way we don't accidentally end up 
> > > > > > > > > > > > > > > with padding between the unnamed bits at the 
> > > > > > > > > > > > > > > start and the named bits in this object).
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > I think adding some assertions would not be a bad 
> > > > > > > > > > > > > > > idea as a follow-up.
> > > > > > > > > > > > > > Maybe some unconditional (rather than only in 
> > > > > > > > > > > > > > asserts builds) error handling? 
> > > > > > > > > > > > > > (report_fatal_error, if this is low priority enough 
> > > > > > > > > > > > > > to not have an elegant failure mode, but something 
> > > > > > > > > > > > > > where we don't just overflow and carry on would be 
> > > > > > > > > > > > > > good... )
> > > > > > > > > > > > > Ping on this? I worry this code has just punted the 
> > > > > > > > > > > > > same bug further down, but not plugged the 
> > > > > > > > > > > > > hole/ensured we don't overflow on novel/larger inputs.
> > > > > > > > > > > > Sorry for the late reply, I was looking through the 
> > > > > > > > > > > > emails and found this. I agree add some assertions to 
> > > > > > > > > > > > check the value is a good idea, It's easy to help 
> > > > > > > > > > > > people catch bugs, at least with when 
> > > > > > > > > > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on 
> > > > > > > > > > > > it, but one thing that worries me is that, in 
> > > > > > > > > > > > ASTReader, we access this field directly, not through 
> > > > > > > > > > > > the constructor or accessor, and we have to add 
> > > > > > > > > > > > assertions everywhere. 
> > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > > > > > I don't think we have to add too many assertions. As best 
> > > > > > > > > > > I can tell, we'll need one in each of the 
> > > > > > > > > > > `PseudoObjectExpr` constructors and one in 
> > > > > > > > > > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are 
> > > > > > > > > > > the only places we assign a value into the bit-field. 
> > > > > > > > > > > Three assertions isn't a lot, but if we're worried, we 
> > > > > > > > > > > could add a setter method that does the assertion and use 
> > > > > > > > > > > the setter in all three places.
> > > > > > > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > > > > > > assertions - but about having a reliable error here. The 
> > > > > > > > > > patch only makes the sizes larger, but doesn't have a 
> > > > > > > > > > hard-stop in case those sizes are exceeded again (which, 
> > > > > > > > > > admittedly, is much harder to do - maybe it's totally 
> > > > > > > > > > unreachable now, for all practical purposes?) 
> > > > > > > > > > 
> > > > > > > > > > I suspect with more carefully constructed recursive inputs 
> > > > > > > > > > could still reach the higher limit & I think it'd be good 
> > > > > > > > > > to fail hard in that case in some way? (it's probably rare 
> > > > > > > > > > enough that a report_fatal_error would be 
> > > > > > > > > > not-the-worst-thing-ever)
> > > > > > > > > > 
> > > > > > > > > > But good assertions would be nice too (the old code only 
> > > > > > > > > > failed when you hit /exactly/ on just the overflow value, 
> > > > > > > > > > and any more 

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

aaron.ballman wrote:
> yronglin wrote:
> > yronglin wrote:
> > > dblaikie wrote:
> > > > yronglin wrote:
> > > > > dblaikie wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > yronglin wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > dblaikie wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > dblaikie wrote:
> > > > > > > > > > > > Could/should we add some error checking in the ctor to 
> > > > > > > > > > > > assert that we don't overflow these longer values/just 
> > > > > > > > > > > > hit the bug later on?
> > > > > > > > > > > > 
> > > > > > > > > > > > (& could we use `unsigned short` here rather than 
> > > > > > > > > > > > bitfields?)
> > > > > > > > > > > We've already got them packed in with other bit-fields 
> > > > > > > > > > > from the expression bits, so I think it's reasonable to 
> > > > > > > > > > > continue the pattern of using bit-fields (that way we 
> > > > > > > > > > > don't accidentally end up with padding between the 
> > > > > > > > > > > unnamed bits at the start and the named bits in this 
> > > > > > > > > > > object).
> > > > > > > > > > > 
> > > > > > > > > > > I think adding some assertions would not be a bad idea as 
> > > > > > > > > > > a follow-up.
> > > > > > > > > > Maybe some unconditional (rather than only in asserts 
> > > > > > > > > > builds) error handling? (report_fatal_error, if this is low 
> > > > > > > > > > priority enough to not have an elegant failure mode, but 
> > > > > > > > > > something where we don't just overflow and carry on would 
> > > > > > > > > > be good... )
> > > > > > > > > Ping on this? I worry this code has just punted the same bug 
> > > > > > > > > further down, but not plugged the hole/ensured we don't 
> > > > > > > > > overflow on novel/larger inputs.
> > > > > > > > Sorry for the late reply, I was looking through the emails and 
> > > > > > > > found this. I agree add some assertions to check the value is a 
> > > > > > > > good idea, It's easy to help people catch bugs, at least with 
> > > > > > > > when `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, 
> > > > > > > > but one thing that worries me is that, in ASTReader, we access 
> > > > > > > > this field directly, not through the constructor or accessor, 
> > > > > > > > and we have to add assertions everywhere. 
> > > > > > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > > > > > I don't think we have to add too many assertions. As best I can 
> > > > > > > tell, we'll need one in each of the `PseudoObjectExpr` 
> > > > > > > constructors and one in `ASTStmtReader::VisitPseudoObjectExpr()`, 
> > > > > > > but those are the only places we assign a value into the 
> > > > > > > bit-field. Three assertions isn't a lot, but if we're worried, we 
> > > > > > > could add a setter method that does the assertion and use the 
> > > > > > > setter in all three places.
> > > > > > My concern wasn't (well, wasn't entirely) about adding more 
> > > > > > assertions - but about having a reliable error here. The patch only 
> > > > > > makes the sizes larger, but doesn't have a hard-stop in case those 
> > > > > > sizes are exceeded again (which, admittedly, is much harder to do - 
> > > > > > maybe it's totally unreachable now, for all practical purposes?) 
> > > > > > 
> > > > > > I suspect with more carefully constructed recursive inputs could 
> > > > > > still reach the higher limit & I think it'd be good to fail hard in 
> > > > > > that case in some way? (it's probably rare enough that a 
> > > > > > report_fatal_error would be not-the-worst-thing-ever)
> > > > > > 
> > > > > > But good assertions would be nice too (the old code only failed 
> > > > > > when you hit /exactly/ on just the overflow value, and any more 
> > > > > > than that the wraparound would not crash/fail, but misbehave) - I 
> > > > > > did add the necessary assertion to ArrayRef (begin <= end) which 
> > > > > > would've helped detect this more reliably, but some assert checking 
> > > > > > for overflow in the ctor would be good too (with all the usual 
> > > > > > nuance/care in checking for overflow) - unless we're going to make 
> > > > > > that into a fatal or other real error.
> > > > > Sorry for the very late reply. I have no preference between assertion 
> > > > > and `llvm_unreachable`, if error then fail fast is looks good. I have 
> > > > > a patch D158296 to add assertion.
> > > > Thanks for the assertions - though they still haven't met my main 
> > > > concern that this 

[PATCH] D158301: Add back overriding-t-options for -m-version-min diagnostic

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/Driver/darwin-version.c:217
 // RUN:   FileCheck --check-prefix=CHECK-VERSION-TNO-OSV1 %s
-// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2'
+// CHECK-VERSION-TNO-OSV1: overriding '-mmacos-version-min=10.6' option with 
'-target x86_64-apple-macos10.11.2' [-Woverriding-t-option]
 

MaskRay wrote:
> dexonsmith wrote:
> > MaskRay wrote:
> > > dexonsmith wrote:
> > > > MaskRay wrote:
> > > > > hans wrote:
> > > > > > MaskRay wrote:
> > > > > > > hans wrote:
> > > > > > > > dexonsmith wrote:
> > > > > > > > > MaskRay wrote:
> > > > > > > > > > dexonsmith wrote:
> > > > > > > > > > > Why would we want to use the old name here? An alias 
> > > > > > > > > > > seems strictly better to me. 
> > > > > > > > > > Making `overriding-t-option` an alias for 
> > > > > > > > > > `overriding-option` would make `-Wno-overriding-t-option` 
> > > > > > > > > > applies to future overriding option diagnostics, which is 
> > > > > > > > > > exactly what I want to avoid.
> > > > > > > > > > 
> > > > > > > > > I understand that you don't want `-t-` to apply to work on 
> > > > > > > > > future overriding option diagnostics, but I think the 
> > > > > > > > > platform divergence you're adding here is worse.
> > > > > > > > > 
> > > > > > > > > Having a few Darwin-specific options use 
> > > > > > > > > `-Woverriding-t-option` (and everything else use 
> > > > > > > > > `-Woverriding-option`) as the canonical spelling is hard to 
> > > > > > > > > reason about for maintainers, and for users.
> > > > > > > > > 
> > > > > > > > > And might not users on other platforms have 
> > > > > > > > > `-Woverriding-t-option` hardcoded in  build settings? (So 
> > > > > > > > > @dblaikie's argument that we shouldn't arbitrarily make 
> > > > > > > > > things hard for users would apply to all options?)
> > > > > > > > > 
> > > > > > > > > IMO, if we're not comfortable removing 
> > > > > > > > > `-Woverriding-t-option` entirely, then it should live on as 
> > > > > > > > > an alias (easy to reason about), not as 
> > > > > > > > > canonical-in-special-cases (hard to reason about).
> > > > > > > > > IMO, if we're not comfortable removing -Woverriding-t-option 
> > > > > > > > > entirely, then it should live on as an alias (easy to reason 
> > > > > > > > > about), not as canonical-in-special-cases (hard to reason 
> > > > > > > > > about).
> > > > > > > > 
> > > > > > > > +1 if we can't drop the old spelling, an alias seems like the 
> > > > > > > > best option.
> > > > > > > Making `overriding-t-option` an alias for `overriding-option`, as 
> > > > > > > I mentioned, will make `-Wno-overriding-t-option` affect new 
> > > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > > > 
> > > > > > > I know there are some `-Wno-overriding-t-option` uses. Honestly, 
> > > > > > > they are far fewer than other diagnostics we are introducing or 
> > > > > > > changing in Clang. I can understand the argument "make -Werror 
> > > > > > > users easier for this specific diagnostic" (but `-Werror` will 
> > > > > > > complain about other new diagnostics), but do we really want to 
> > > > > > > in the Darwin case? I think no. They can remove the version from 
> > > > > > > the target triple like 
> > > > > > > https://github.com/facebook/ocamlrep/blame/abc14b8aafcc6746ec37bf7bf0de24bfc58d63a0/prelude/apple/apple_target_sdk_version.bzl#L50
> > > > > > > 　or make the version part consistent with `-m.*os-version-min`.
> > > > > > > 
> > > > > > > This change may force these users to re-think how they should fix 
> > > > > > >  their build. I apology to these users, but I don't feel that 
> > > > > > > adding an alias is really necessary.
> > > > > > > Making overriding-t-option an alias for overriding-option, as I 
> > > > > > > mentioned, will make -Wno-overriding-t-option affect new 
> > > > > > > overriding-options uses. This is exactly what I want to avoid.
> > > > > > 
> > > > > > Is keeping them separate actually important, though? 
> > > > > > -Wno-overriding-option has the same issue in that case, that using 
> > > > > > the flag will also affect any new overriding-options uses, and I 
> > > > > > don't think that's a problem.
> > > > > `-Wno-overriding-option` is properly named, so affecting new 
> > > > > overriding-options uses looks fine to me.
> > > > > `-Wno-overriding-t-option` is awkward, and making it affect new uses 
> > > > > makes me nervous.
> > > > > 
> > > > > The gist of my previous comment is whether the uses cases really 
> > > > > justify a tiny bit of tech bit in clang and I think the answer is no.
> > > > > 
> > > > > This change is not about changing a semantic warning that has mixed 
> > > > > opinions, e.g. `-Wbitwise-op-parentheses` (many consider it not 
> > > > > justified).
> > > > > The gist of my previous comment is whether the uses cases really 
> 

[PATCH] D157762: [WIP] Implement [[msvc::no_unique_address]]

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/AST/Decl.cpp:4523-4524
 bool FieldDecl::isPotentiallyOverlapping() const {
-  return hasAttr() && getType()->getAsCXXRecordDecl();
+  return (hasAttr() ||
+  hasAttr()) &&
+ getType()->getAsCXXRecordDecl();

akhuang wrote:
> aaron.ballman wrote:
> > akhuang wrote:
> > > aaron.ballman wrote:
> > > > dblaikie wrote:
> > > > > Having to check both of these in several places seems problematic - 
> > > > > can we wrap that up somewhere? (or, maybe ideally, is there a way for 
> > > > > `msvc::no_unique_address` to map to the actual NoUniqueAddressAttr as 
> > > > > a different spelling of the same thing?)
> > > > This was why I was hoping we could merge the two in Attr.td, but I'm 
> > > > not certain that will be easy.
> > > What does merging the two in Attr.td mean? Could we just put the two 
> > > spellings in one attribute, or would that make it impossible for clang-cl 
> > > to ignore the [[no_unique_address]] spelling
> > We can have multiple syntactic spellings for the same semantic attribute 
> > (e.g., `__attribute__((foo))` and `__attribute__((bar))` can both map to a 
> > single `FooBarAttr` AST node), and we have "accessors" on the AST node that 
> > let you tell which spelling was used: 
> > https://github.com/llvm/llvm-project/blob/90ecadde62f30275c35fdf7928e3477a41691d21/clang/include/clang/Basic/Attr.td#L4095
> > 
> > The suggestion Erich and I are thinking of is:
> > 
> > 1) Add the additional spelling to `NoUniqueAddress`.
> > 2) Add accessors to differentiate the spellings.
> > 3) Remove the `TargetSpecificAttr` from `NoUniqueAddress`, manually 
> > implement those checks in an attribute handler in SemaDeclAttr.cpp.
> > 
> > Then, anywhere you care about the attribute in general, you can look for 
> > `isa`, and anywhere you care about which spelling, you can 
> > use `cast(A)->isMSVC()` (or whatever you name the 
> > accessors).
> Thanks! This patch should implement this, and so far, I don't think there are 
> any places outside of SemaDeclAttr where we have to differentiate the two.
(awesome - glad to see this was so tidy to do/that the generic attribute 
handling stuff mostly covers it)



Comment at: clang/lib/AST/Decl.cpp:4510
+  if (Ctx.getTargetInfo().getCXXABI().isMicrosoft() &&
+  llvm::any_of(CXXRD->fields(), [&](const FieldDecl *Field) {
+return Field->getType()->getAs();





Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:3096-3098
+  if (!IsOverlappingEmptyField) {
+DataSize = std::max(DataSize, FieldOffset + Info.Size);
+  }




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157762/new/

https://reviews.llvm.org/D157762

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

[PATCH] D158296: [NFC][Clang] Add assertion to check the value of NumSubExprs/ResultIndex does not overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Hopefully my clarification on https://reviews.llvm.org/D154784#inline-1531176 
makes it clear that assertions aren't necessarily the right tool here - as this 
path is reachable with actual code & I don't think we should allow that code to 
reach UB in the compiler in a non-assertionsn build. If we think this is a 
sufficient size to support, then we should error clearly even in a 
non-assertions build.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158296/new/

https://reviews.llvm.org/D158296

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

yronglin wrote:
> dblaikie wrote:
> > aaron.ballman wrote:
> > > yronglin wrote:
> > > > dblaikie wrote:
> > > > > dblaikie wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > dblaikie wrote:
> > > > > > > > Could/should we add some error checking in the ctor to assert 
> > > > > > > > that we don't overflow these longer values/just hit the bug 
> > > > > > > > later on?
> > > > > > > > 
> > > > > > > > (& could we use `unsigned short` here rather than bitfields?)
> > > > > > > We've already got them packed in with other bit-fields from the 
> > > > > > > expression bits, so I think it's reasonable to continue the 
> > > > > > > pattern of using bit-fields (that way we don't accidentally end 
> > > > > > > up with padding between the unnamed bits at the start and the 
> > > > > > > named bits in this object).
> > > > > > > 
> > > > > > > I think adding some assertions would not be a bad idea as a 
> > > > > > > follow-up.
> > > > > > Maybe some unconditional (rather than only in asserts builds) error 
> > > > > > handling? (report_fatal_error, if this is low priority enough to 
> > > > > > not have an elegant failure mode, but something where we don't just 
> > > > > > overflow and carry on would be good... )
> > > > > Ping on this? I worry this code has just punted the same bug further 
> > > > > down, but not plugged the hole/ensured we don't overflow on 
> > > > > novel/larger inputs.
> > > > Sorry for the late reply, I was looking through the emails and found 
> > > > this. I agree add some assertions to check the value is a good idea, 
> > > > It's easy to help people catch bugs, at least with when 
> > > > `-DLLVM_ENABLE_ASSERTIONS=ON`, and I'm glad to work on it, but one 
> > > > thing that worries me is that, in ASTReader, we access this field 
> > > > directly, not through the constructor or accessor, and we have to add 
> > > > assertions everywhere. 
> > > > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> > > I don't think we have to add too many assertions. As best I can tell, 
> > > we'll need one in each of the `PseudoObjectExpr` constructors and one in 
> > > `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only places 
> > > we assign a value into the bit-field. Three assertions isn't a lot, but 
> > > if we're worried, we could add a setter method that does the assertion 
> > > and use the setter in all three places.
> > My concern wasn't (well, wasn't entirely) about adding more assertions - 
> > but about having a reliable error here. The patch only makes the sizes 
> > larger, but doesn't have a hard-stop in case those sizes are exceeded again 
> > (which, admittedly, is much harder to do - maybe it's totally unreachable 
> > now, for all practical purposes?) 
> > 
> > I suspect with more carefully constructed recursive inputs could still 
> > reach the higher limit & I think it'd be good to fail hard in that case in 
> > some way? (it's probably rare enough that a report_fatal_error would be 
> > not-the-worst-thing-ever)
> > 
> > But good assertions would be nice too (the old code only failed when you 
> > hit /exactly/ on just the overflow value, and any more than that the 
> > wraparound would not crash/fail, but misbehave) - I did add the necessary 
> > assertion to ArrayRef (begin <= end) which would've helped detect this more 
> > reliably, but some assert checking for overflow in the ctor would be good 
> > too (with all the usual nuance/care in checking for overflow) - unless 
> > we're going to make that into a fatal or other real error.
> Sorry for the very late reply. I have no preference between assertion and 
> `llvm_unreachable`, if error then fail fast is looks good. I have a patch 
> D158296 to add assertion.
Thanks for the assertions - though they still haven't met my main concern that 
this should have a hard failure even in a non-assertions build.

I know we don't have a perfect plan/policy for these sort of "run out of 
resources/hit a representational limit" issues (at least I don't think we do... 
do we, @aaron.ballman ? I know we have some limits (recursion, template 
expansion, etc) but they're fairly specific/aren't about every possible case of 
integer overflow in some representational element, etc) but we've seen this one 
is pretty reachable. 

Here's a test case that would still trigger the assertion, and trigger UB in a 
non-assertions build:
```
truct t1 { };
template
struct templ {
T1 v1;
T1 v2;
T1 v3;
T1 v4;
};

struct t2 {
  templ> c0;
  

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D158137#4599481 , @MaskRay wrote:

> In D158137#4599461 , @dblaikie 
> wrote:
>
>> In D158137#4597491 , @dexonsmith 
>> wrote:
>>
>>> In D158137#4597009 , @MaskRay 
>>> wrote:
>>>
 In D158137#4596948 , @dexonsmith 
 wrote:

> Can you explain the downside of leaving behind an alias?

 Two minor ones. (a) Existing `-Wno-overriding-t-option` will not notice 
 that they need to migrate and (b) Clang has accrued tiny tech debt.
 If we eventually remove `-Wno-overriding-t-option` for tidiness, we will 
 have to break `-Werror -Wno-overriding-t-option` users.
>>>
>>> I guess it's not clear to me we'd need to remove the alias. The usual 
>>> policy (I think?) is that clang driver options don't disappear. It seems 
>>> like a small piece of debt to maintain the extra alias in this case, and if 
>>> it's kept, then users don't actually need to migrate. And then you can feel 
>>> safe updating Darwin.cpp as well.
>>
>> +1 to this, FWIW - I wouldn't consider it technical debt to keep a 
>> compatible warning flag name that's been around for a decade & isn't a name 
>> we're trying to free up for some other use or because it causes any great 
>> confusion, etc.
>
> I think my previous comment has answered this.
> Think: every Clang release may emit some new warnings. `-Werror` users has 
> actually provided a promise that they will fix reasonable toolchain changes. 
> Toolchain contributors check how disruptive a change will be, but cannot 
> promise that we never emit new warnings.
> In this case, `overriding-t-options` seems a fairly rare and LLVM/Clang side 
> has far too many other fp-model/fast-math changes to make "whether we will 
> get a new warning" a fairly minor issue.
> I am unfamiliar with Darwin.cpp though. If it turns out that want to disable 
> the warning even with `-Wno-overriding-t-option`, we can add a workaround 
> specific to Darwin.cpp, but not to fp-model/Tc.

Sure, I understand that we can break these things - like if we totally remove a 
warning, I wouldn't be averse to removing the flag/not leaving it there for 
backwards "compatibility" (when it's misleading/doesn't actually trigger the 
warning, for instance). But in this case it seems like we're keeping the 
functionality, decided on a better name, but it seems pretty harmless and 
somewhat beneficial to keep the old name around.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158137/new/

https://reviews.llvm.org/D158137

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

[PATCH] D158137: Rename warn_drv_overriding_flag_option (-Woverriding-t-option) to warn_drv_overriding_flag_option (-Woverriding-option)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D158137#4597491 , @dexonsmith 
wrote:

> In D158137#4597009 , @MaskRay wrote:
>
>> In D158137#4596948 , @dexonsmith 
>> wrote:
>>
>>> Can you explain the downside of leaving behind an alias?
>>
>> Two minor ones. (a) Existing `-Wno-overriding-t-option` will not notice that 
>> they need to migrate and (b) Clang has accrued tiny tech debt.
>> If we eventually remove `-Wno-overriding-t-option` for tidiness, we will 
>> have to break `-Werror -Wno-overriding-t-option` users.
>
> I guess it's not clear to me we'd need to remove the alias. The usual policy 
> (I think?) is that clang driver options don't disappear. It seems like a 
> small piece of debt to maintain the extra alias in this case, and if it's 
> kept, then users don't actually need to migrate. And then you can feel safe 
> updating Darwin.cpp as well.

+1 to this, FWIW - I wouldn't consider it technical debt to keep a compatible 
warning flag name that's been around for a decade & isn't a name we're trying 
to free up for some other use or because it causes any great confusion, etc.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158137/new/

https://reviews.llvm.org/D158137

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D156726#4594005 , @yonghong-song 
wrote:

> In D156726#4593671 , @dblaikie 
> wrote:
>
>> Fixed in 19c216b5ab7c34c297b4c3c4da64b49b3c830a3a
>
> Hi, @dblaikie I cannot find the above commit in llvm-project and with latest 
> llvm-project 'main' branch, the problem seems still exist.

Ah, right right - sorry, failed to push. Here it is: 
2993243c45abdb4f2bc3979336d054be165b1134 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Fixed in 19c216b5ab7c34c297b4c3c4da64b49b3c830a3a


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D158006: [Clang][WIP]Experimental implementation of data member packs in dependent context.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaExprMember.cpp:519-521
+  if (const ElaboratedType *ET = dyn_cast(BaseType)) {
+if (const TemplateSpecializationType *TST =
+dyn_cast(ET->getNamedType())) {

I think you can skip the `const` in the first `dyn_cast` (think it works 
implicitly?) & could use `const auto *` on the LHS of both these `if`s, since 
the RHS makes it clear what the type is



Comment at: clang/lib/Sema/SemaExprMember.cpp:523-524
+  auto *TD = TST->getTemplateName().getAsTemplateDecl();
+  assert(isa(TD) &&
+ "template decl in member access is not ClassTemplateDecl");
+  for (FieldDecl *Field :

No need for the assert if you're immediately going to `cast` anyway, it'll 
assert. Though perhaps the custom assert here gives you a chance to make it 
more explicit that this is intentional.



Comment at: clang/lib/Sema/SemaExprMember.cpp:525-527
+  for (FieldDecl *Field :
+   cast(TD)->getTemplatedDecl()->fields()) {
+if (Field->getDeclName() == NameInfo.getName()) {

This could be `llvm::find_if`, maybe? Not sure if it'd be tidier, but maybe 
more legible



Comment at: clang/lib/Sema/SemaExprMember.cpp:1023-1025
+if (MemberNameStr.find('@') != std::string::npos) {
+  return ExprEmpty();
+}

Usually don't put `{}` on single-line scopes in the LLVM codebase.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3292
+  std::string NewFieldName =
+  PackedField->getName().str() + "@" + std::to_string(Arg);
+  PackedField->setDeclName((NewFieldName));

cjdb wrote:
> Does LLVM have some form of `absl::StrCat` that we can use instead of 
> `operator+`?
there's at least `llvm::Twine` which can avoid manifesting the intermediate 
strings



Comment at: clang/lib/Sema/TreeTransform.h:4171
 
+  if (CXXDependentScopeMemberExpr *MemberExpr =
+  dyn_cast(Pattern)) {

Might be worth pulling out this new code as a separate function - the 
`continue` at the end is many lines away from the start or end of the loop, 
making control flow a bit hard to follow (probably the existing code could do 
with some of this massaging even before/regardless of the new code you're 
adding here)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158006/new/

https://reviews.llvm.org/D158006

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D156726#4591543 , @eddyz87 wrote:

> As an additional data point, the same example but w/o section specification 
> compiles fine:
>
>   const int with_init = 1;
>   const int no_init;
>
> And puts both globals to the same section:
>
>   $ clang -c t.c -o - | llvm-readelf --section-headers -s -
>   Section Headers:
> [Nr] Name  TypeAddress  OffSize   ES 
> Flg Lk Inf Al
> ...
> [ 3] .rodata   PROGBITS 40 08 00  
>  A  0   0  4
> ...
>   
>   Symbol table '.symtab' contains 4 entries:
>  Num:Value  Size TypeBind   Vis   Ndx Name
>...
>2:  4 OBJECT  GLOBAL DEFAULT 3 with_init
>3: 0004 4 OBJECT  GLOBAL DEFAULT 3 no_init
>
> (`Ndx` stands for section `Nr`).

Thanks for the details - looking into it!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds OK to me - thanks!

bit more detail in the actual patch description/commit message (from some of 
the comments in this review, perhaps) would be handy.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156571/new/

https://reviews.llvm.org/D156571

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG19f2b68095fe: Make globals with mutable members 
non-constant, even in custom sections (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Type.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/Targets/AMDGPU.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/sections.cpp
  clang/test/SemaCXX/attr-section.cpp

Index: clang/test/SemaCXX/attr-section.cpp
===
--- clang/test/SemaCXX/attr-section.cpp
+++ clang/test/SemaCXX/attr-section.cpp
@@ -48,3 +48,24 @@
 const int const_global_var __attribute__((section("template_fn1"))) = 42;   // expected-error {{'const_global_var' causes a section type conflict with 'template_fn1'}}
 int mut_global_var __attribute__((section("template_fn2"))) = 42;   // expected-note {{declared here}}
 template  __attribute__((section("template_fn2"))) void template_fn2() {} // expected-error {{'template_fn2' causes a section type conflict with 'mut_global_var'}}
+
+namespace mutable_member {
+struct t1 {
+  mutable int i;
+};
+extern const t1 v1;
+__attribute__((section("mutable_member"))) const t1 v1{};
+extern int i;
+__attribute__((section("mutable_member"))) int i{};
+} // namespace mutable_member
+
+namespace non_trivial_ctor {
+struct t1 {
+  t1();
+  constexpr t1(int) { }
+};
+extern const t1 v1;
+__attribute__((section("non_trivial_ctor"))) const t1 v1; // expected-note {{declared here}}
+extern const t1 v2;
+__attribute__((section("non_trivial_ctor"))) const t1 v2{3}; // expected-error {{'v2' causes a section type conflict with 'v1'}}
+} // namespace non_trivial_ctor
Index: clang/test/CodeGenCXX/sections.cpp
===
--- clang/test/CodeGenCXX/sections.cpp
+++ clang/test/CodeGenCXX/sections.cpp
@@ -1,12 +1,36 @@
-// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -verify -o - %s | FileCheck %s
 
 extern "C" {
 
+struct Mutable {
+mutable int i = 3;
+};
+extern const Mutable mutable_default_section;
+const Mutable mutable_default_section;
+struct Normal {
+  int i = 2;
+};
+extern const Normal normal_default_section;
+const Normal normal_default_section;
 #pragma const_seg(".my_const")
 #pragma bss_seg(".my_bss")
 int D = 1;
 #pragma data_seg(".data")
 int a = 1;
+extern const Mutable mutable_custom_section;
+const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'mutable_custom_section' due to the presence of a mutable field}}
+extern const Normal normal_custom_section;
+const Normal normal_custom_section;
+struct NonTrivialDtor {
+  ~NonTrivialDtor();
+};
+extern const NonTrivialDtor non_trivial_dtor_custom_section;
+const NonTrivialDtor non_trivial_dtor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_dtor_custom_section' due to the presence of a non-trivial destructor}}
+struct NonTrivialCtor {
+  NonTrivialCtor();
+};
+extern const NonTrivialCtor non_trivial_ctor_custom_section;
+const NonTrivialCtor non_trivial_ctor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_ctor_custom_section' due to the presence of a non-trivial constructor}}
 #pragma data_seg(push, label, ".data2")
 extern const int b;
 const int b = 1;
@@ -74,10 +98,19 @@
 #pragma section("short_section", short)
 // Pragma section ignores "short".
 __declspec(allocate("short_section")) short short_var = 42;
+
+struct t2 { t2(); };
+extern const t2 non_trivial_ctor;
+__declspec(allocate("non_trivial_ctor_section")) const t2 non_trivial_ctor_var;
 }
 
+
+//CHECK: @mutable_default_section = dso_local global %struct.Mutable { i32 3 }, align 4{{$}}
+//CHECK: @normal_default_section = dso_local constant %struct.Normal { i32 2 }, align 4{{$}}
 //CHECK: @D = dso_local global i32 1
 //CHECK: @a = dso_local global i32 1, section ".data"
+//CHECK: @mutable_custom_section = dso_local global %struct.Mutable { i32 3 }, section ".data", align 4
+//CHECK: @normal_custom_section = dso_local constant %struct.Normal { i32 2 }, section ".my_const", align 4
 //CHECK: @b = dso_local constant i32 1, section ".my_const"
 //CHECK: @[[MYSTR:.*]] = {{.*}} unnamed_addr constant [11 x i8] c"my 

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14254
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)

rnk wrote:
> efriedma wrote:
> > dblaikie wrote:
> > > efriedma wrote:
> > > > dblaikie wrote:
> > > > > rnk wrote:
> > > > > > rsmith wrote:
> > > > > > > efriedma wrote:
> > > > > > > > rnk wrote:
> > > > > > > > > I think this is not compatible with MSVC. MSVC uses simple 
> > > > > > > > > logic, it doesn't look for mutable: 
> > > > > > > > > https://gcc.godbolt.org/z/sj6d4saxx
> > > > > > > > > 
> > > > > > > > > The const mutable struct appears in the myrdata section in 
> > > > > > > > > that example.
> > > > > > > > > 
> > > > > > > > > I think the solution is to separate the flag logic from the 
> > > > > > > > > pragma stack selection logic, which has to remain 
> > > > > > > > > MSVC-compatible.
> > > > > > > > MSVC apparently looks at whether the variable is marked 
> > > > > > > > "const", and nothing else; it doesn't look at mutable, it 
> > > > > > > > doesn't look at whether the variable has a constant 
> > > > > > > > initializer.  So the current code isn't right either; if we're 
> > > > > > > > trying to implement MSVC-compatible logic, we shouldn't check 
> > > > > > > > HasConstInit.
> > > > > > > > 
> > > > > > > > That said, I'm not sure how precisely/in what modes we want to 
> > > > > > > > precisely emulate MSVC.  Probably anything we do here will be 
> > > > > > > > confusing.
> > > > > > > We should at least issue a warning if the sensible logic and the 
> > > > > > > MSVC-compatible calculation differ. @rnk, do you know how 
> > > > > > > important it is to follow the MSVC semantics in this regard?
> > > > > > I think it depends on whether you think that users are primarily 
> > > > > > using these pragmas to override the default rdata/bss/data sections 
> > > > > > without any care about precisely what goes where, or if they are 
> > > > > > using them to do something finer grained.
> > > > > > 
> > > > > > If I had to guess, I'd say it's more likely the former, given that 
> > > > > > `__declspec(allocate)` and `#pragma(section)` exist to handle cases 
> > > > > > where users are putting specific globals into specific sections.
> > > > > > 
> > > > > > Which, if we follow Richard's suggestion, would mean warning when 
> > > > > > we put a global marked `const` into a writable section when 
> > > > > > `ConstSegStack` is non-empty. That seems reasonable. 
> > > > > > `-Wmicrosoft-const-seg` for the new warning group?
> > > > > Does the MSVC situation only apply to custom sections? (presumably 
> > > > > when not customizing the section, MSVC gets it right and can support 
> > > > > a const global with a runtime initializer, mutable member, or 
> > > > > mutating dtor?)
> > > > > 
> > > > > I think this code still needs to be modified, since this is the code 
> > > > > that drives the error about incompatible sections. So it'll need to 
> > > > > behave differently depending on the target platform?
> > > > Yes, the MSVC situation is specifically if you specify `#pragma 
> > > > const_seg`; without the pragma, it does what you'd expect.
> > > Went with the "let's do the thing that the user probably wants, but isn't 
> > > what MSVC does, and warn when that difference comes up" - if that's OK 
> > > with everyone.
> > > 
> > > (always open to wordsmithing the warning - and if we want to, can go to 
> > > the extra layer and specifically diagnose which reason (mutable members, 
> > > non-const init) - and I can't quite figure out the best phrasing to say 
> > > "we're putting it in section X insetad of section Y, because Z, but 
> > > Microsoft would use X because A" or something... it's all a bit of a 
> > > mouthful)
> > Describing which reason actually applies would make the warning a lot 
> > easier to read.
> That is true, but I think very few people will see this diagnostic. I'm not 
> sure it's worth the added code complexity to implement that improvement.
Updated with a specific diagnosis. Phrasing still feels a bit awkward/I'm open 
to wordsmithing suggestions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 550097.
dblaikie added a comment.

Diagnose specific reasons


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Type.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/Targets/AMDGPU.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/sections.cpp
  clang/test/SemaCXX/attr-section.cpp

Index: clang/test/SemaCXX/attr-section.cpp
===
--- clang/test/SemaCXX/attr-section.cpp
+++ clang/test/SemaCXX/attr-section.cpp
@@ -48,3 +48,24 @@
 const int const_global_var __attribute__((section("template_fn1"))) = 42;   // expected-error {{'const_global_var' causes a section type conflict with 'template_fn1'}}
 int mut_global_var __attribute__((section("template_fn2"))) = 42;   // expected-note {{declared here}}
 template  __attribute__((section("template_fn2"))) void template_fn2() {} // expected-error {{'template_fn2' causes a section type conflict with 'mut_global_var'}}
+
+namespace mutable_member {
+struct t1 {
+  mutable int i;
+};
+extern const t1 v1;
+__attribute__((section("mutable_member"))) const t1 v1{};
+extern int i;
+__attribute__((section("mutable_member"))) int i{};
+} // namespace mutable_member
+
+namespace non_trivial_ctor {
+struct t1 {
+  t1();
+  constexpr t1(int) { }
+};
+extern const t1 v1;
+__attribute__((section("non_trivial_ctor"))) const t1 v1; // expected-note {{declared here}}
+extern const t1 v2;
+__attribute__((section("non_trivial_ctor"))) const t1 v2{3}; // expected-error {{'v2' causes a section type conflict with 'v1'}}
+} // namespace non_trivial_ctor
Index: clang/test/CodeGenCXX/sections.cpp
===
--- clang/test/CodeGenCXX/sections.cpp
+++ clang/test/CodeGenCXX/sections.cpp
@@ -1,12 +1,36 @@
-// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -verify -o - %s | FileCheck %s
 
 extern "C" {
 
+struct Mutable {
+mutable int i = 3;
+};
+extern const Mutable mutable_default_section;
+const Mutable mutable_default_section;
+struct Normal {
+  int i = 2;
+};
+extern const Normal normal_default_section;
+const Normal normal_default_section;
 #pragma const_seg(".my_const")
 #pragma bss_seg(".my_bss")
 int D = 1;
 #pragma data_seg(".data")
 int a = 1;
+extern const Mutable mutable_custom_section;
+const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'mutable_custom_section' due to the presence of a mutable field}}
+extern const Normal normal_custom_section;
+const Normal normal_custom_section;
+struct NonTrivialDtor {
+  ~NonTrivialDtor();
+};
+extern const NonTrivialDtor non_trivial_dtor_custom_section;
+const NonTrivialDtor non_trivial_dtor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_dtor_custom_section' due to the presence of a non-trivial destructor}}
+struct NonTrivialCtor {
+  NonTrivialCtor();
+};
+extern const NonTrivialCtor non_trivial_ctor_custom_section;
+const NonTrivialCtor non_trivial_ctor_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'non_trivial_ctor_custom_section' due to the presence of a non-trivial constructor}}
 #pragma data_seg(push, label, ".data2")
 extern const int b;
 const int b = 1;
@@ -74,10 +98,19 @@
 #pragma section("short_section", short)
 // Pragma section ignores "short".
 __declspec(allocate("short_section")) short short_var = 42;
+
+struct t2 { t2(); };
+extern const t2 non_trivial_ctor;
+__declspec(allocate("non_trivial_ctor_section")) const t2 non_trivial_ctor_var;
 }
 
+
+//CHECK: @mutable_default_section = dso_local global %struct.Mutable { i32 3 }, align 4{{$}}
+//CHECK: @normal_default_section = dso_local constant %struct.Normal { i32 2 }, align 4{{$}}
 //CHECK: @D = dso_local global i32 1
 //CHECK: @a = dso_local global i32 1, section ".data"
+//CHECK: @mutable_custom_section = dso_local global %struct.Mutable { i32 3 }, section ".data", align 4
+//CHECK: @normal_custom_section = dso_local constant %struct.Normal { i32 2 }, section ".my_const", align 4
 //CHECK: @b = dso_local constant i32 1, section ".my_const"
 //CHECK: @[[MYSTR:.*]] = {{.*}} unnamed_addr constant [11 x i8] c"my string!\00"
 //CHECK: @s = dso_local global ptr @[[MYSTR]], section ".data2"
@@ -96,6 +129,7 @@
 //CHECK: @implicitly_read_write = dso_local global i32 42, section 

[PATCH] D157762: [WIP] Implement [[msvc::no_unique_address]]

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/AST/Decl.cpp:4510-4512
+for (const FieldDecl *Field : CXXRD->fields())
+  if (Field->getType()->getAsCXXRecordDecl())
+return false;

maybe `llvm::any_of`? Not especially shorter, but maybe makes it a smidge 
easier to read?



Comment at: clang/lib/AST/Decl.cpp:4523-4524
 bool FieldDecl::isPotentiallyOverlapping() const {
-  return hasAttr() && getType()->getAsCXXRecordDecl();
+  return (hasAttr() ||
+  hasAttr()) &&
+ getType()->getAsCXXRecordDecl();

Having to check both of these in several places seems problematic - can we wrap 
that up somewhere? (or, maybe ideally, is there a way for 
`msvc::no_unique_address` to map to the actual NoUniqueAddressAttr as a 
different spelling of the same thing?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157762/new/

https://reviews.llvm.org/D157762

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@rnk & I figured that the user probably isn't relying on this much - if they 
cared about a particular variable, they could use an attribute instead of a 
pragma. The pragma just being about "put all my const things over here and all 
my non-const things over there" - bug-for-bug compatibility doesn't seem /as/ 
needed here.

Not sure how you feel about it, @efriedma ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14254
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)

efriedma wrote:
> dblaikie wrote:
> > rnk wrote:
> > > rsmith wrote:
> > > > efriedma wrote:
> > > > > rnk wrote:
> > > > > > I think this is not compatible with MSVC. MSVC uses simple logic, 
> > > > > > it doesn't look for mutable: https://gcc.godbolt.org/z/sj6d4saxx
> > > > > > 
> > > > > > The const mutable struct appears in the myrdata section in that 
> > > > > > example.
> > > > > > 
> > > > > > I think the solution is to separate the flag logic from the pragma 
> > > > > > stack selection logic, which has to remain MSVC-compatible.
> > > > > MSVC apparently looks at whether the variable is marked "const", and 
> > > > > nothing else; it doesn't look at mutable, it doesn't look at whether 
> > > > > the variable has a constant initializer.  So the current code isn't 
> > > > > right either; if we're trying to implement MSVC-compatible logic, we 
> > > > > shouldn't check HasConstInit.
> > > > > 
> > > > > That said, I'm not sure how precisely/in what modes we want to 
> > > > > precisely emulate MSVC.  Probably anything we do here will be 
> > > > > confusing.
> > > > We should at least issue a warning if the sensible logic and the 
> > > > MSVC-compatible calculation differ. @rnk, do you know how important it 
> > > > is to follow the MSVC semantics in this regard?
> > > I think it depends on whether you think that users are primarily using 
> > > these pragmas to override the default rdata/bss/data sections without any 
> > > care about precisely what goes where, or if they are using them to do 
> > > something finer grained.
> > > 
> > > If I had to guess, I'd say it's more likely the former, given that 
> > > `__declspec(allocate)` and `#pragma(section)` exist to handle cases where 
> > > users are putting specific globals into specific sections.
> > > 
> > > Which, if we follow Richard's suggestion, would mean warning when we put 
> > > a global marked `const` into a writable section when `ConstSegStack` is 
> > > non-empty. That seems reasonable. `-Wmicrosoft-const-seg` for the new 
> > > warning group?
> > Does the MSVC situation only apply to custom sections? (presumably when not 
> > customizing the section, MSVC gets it right and can support a const global 
> > with a runtime initializer, mutable member, or mutating dtor?)
> > 
> > I think this code still needs to be modified, since this is the code that 
> > drives the error about incompatible sections. So it'll need to behave 
> > differently depending on the target platform?
> Yes, the MSVC situation is specifically if you specify `#pragma const_seg`; 
> without the pragma, it does what you'd expect.
Went with the "let's do the thing that the user probably wants, but isn't what 
MSVC does, and warn when that difference comes up" - if that's OK with everyone.

(always open to wordsmithing the warning - and if we want to, can go to the 
extra layer and specifically diagnose which reason (mutable members, non-const 
init) - and I can't quite figure out the best phrasing to say "we're putting it 
in section X insetad of section Y, because Z, but Microsoft would use X because 
A" or something... it's all a bit of a mouthful)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 549206.
dblaikie added a comment.

Add a warning for MSVC pragma inconsistency


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

Files:
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/Type.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/Targets/AMDGPU.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/sections.cpp
  clang/test/SemaCXX/attr-section.cpp

Index: clang/test/SemaCXX/attr-section.cpp
===
--- clang/test/SemaCXX/attr-section.cpp
+++ clang/test/SemaCXX/attr-section.cpp
@@ -48,3 +48,24 @@
 const int const_global_var __attribute__((section("template_fn1"))) = 42;   // expected-error {{'const_global_var' causes a section type conflict with 'template_fn1'}}
 int mut_global_var __attribute__((section("template_fn2"))) = 42;   // expected-note {{declared here}}
 template  __attribute__((section("template_fn2"))) void template_fn2() {} // expected-error {{'template_fn2' causes a section type conflict with 'mut_global_var'}}
+
+namespace mutable_member {
+struct t1 {
+  mutable int i;
+};
+extern const t1 v1;
+__attribute__((section("mutable_member"))) const t1 v1{};
+extern int i;
+__attribute__((section("mutable_member"))) int i{};
+} // namespace mutable_member
+
+namespace non_trivial_ctor {
+struct t1 {
+  t1();
+  constexpr t1(int) { }
+};
+extern const t1 v1;
+__attribute__((section("non_trivial_ctor"))) const t1 v1; // expected-note {{declared here}}
+extern const t1 v2;
+__attribute__((section("non_trivial_ctor"))) const t1 v2{3}; // expected-error {{'v2' causes a section type conflict with 'v1'}}
+} // namespace non_trivial_ctor
Index: clang/test/CodeGenCXX/sections.cpp
===
--- clang/test/CodeGenCXX/sections.cpp
+++ clang/test/CodeGenCXX/sections.cpp
@@ -1,12 +1,26 @@
-// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple i686-pc-win32 -fms-extensions -verify -o - %s | FileCheck %s
 
 extern "C" {
 
+struct Mutable {
+mutable int i = 3;
+};
+extern const Mutable mutable_default_section;
+const Mutable mutable_default_section;
+struct Normal {
+  int i = 2;
+};
+extern const Normal normal_default_section;
+const Normal normal_default_section;
 #pragma const_seg(".my_const")
 #pragma bss_seg(".my_bss")
 int D = 1;
 #pragma data_seg(".data")
 int a = 1;
+extern const Mutable mutable_custom_section;
+const Mutable mutable_custom_section; // expected-warning {{`#pragma const_seg` for section ".my_const" will not apply to 'mutable_custom_section' due to non-trivial initialization or member mutability}}
+extern const Normal normal_custom_section;
+const Normal normal_custom_section;
 #pragma data_seg(push, label, ".data2")
 extern const int b;
 const int b = 1;
@@ -74,10 +88,19 @@
 #pragma section("short_section", short)
 // Pragma section ignores "short".
 __declspec(allocate("short_section")) short short_var = 42;
+
+struct t2 { t2(); };
+extern const t2 non_trivial_ctor;
+__declspec(allocate("non_trivial_ctor_section")) const t2 non_trivial_ctor_var;
 }
 
+
+//CHECK: @mutable_default_section = dso_local global %struct.Mutable { i32 3 }, align 4{{$}}
+//CHECK: @normal_default_section = dso_local constant %struct.Normal { i32 2 }, align 4{{$}}
 //CHECK: @D = dso_local global i32 1
 //CHECK: @a = dso_local global i32 1, section ".data"
+//CHECK: @mutable_custom_section = dso_local global %struct.Mutable { i32 3 }, section ".data", align 4
+//CHECK: @normal_custom_section = dso_local constant %struct.Normal { i32 2 }, section ".my_const", align 4
 //CHECK: @b = dso_local constant i32 1, section ".my_const"
 //CHECK: @[[MYSTR:.*]] = {{.*}} unnamed_addr constant [11 x i8] c"my string!\00"
 //CHECK: @s = dso_local global ptr @[[MYSTR]], section ".data2"
@@ -96,6 +119,7 @@
 //CHECK: @implicitly_read_write = dso_local global i32 42, section "no_section_attributes"
 //CHECK: @long_var = dso_local global i32 42, section "long_section"
 //CHECK: @short_var = dso_local global i16 42, section "short_section"
+//CHECK: @non_trivial_ctor_var = internal global %struct.t2 zeroinitializer, section "non_trivial_ctor_section"
 //CHECK: define dso_local void @g()
 //CHECK: define dso_local void @h() {{.*}} section ".my_code"
 //CHECK: define dso_local void @h2() {{.*}} section ".my_code"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14360,19 +14360,19 @@
  

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:14254
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)

rnk wrote:
> rsmith wrote:
> > efriedma wrote:
> > > rnk wrote:
> > > > I think this is not compatible with MSVC. MSVC uses simple logic, it 
> > > > doesn't look for mutable: https://gcc.godbolt.org/z/sj6d4saxx
> > > > 
> > > > The const mutable struct appears in the myrdata section in that example.
> > > > 
> > > > I think the solution is to separate the flag logic from the pragma 
> > > > stack selection logic, which has to remain MSVC-compatible.
> > > MSVC apparently looks at whether the variable is marked "const", and 
> > > nothing else; it doesn't look at mutable, it doesn't look at whether the 
> > > variable has a constant initializer.  So the current code isn't right 
> > > either; if we're trying to implement MSVC-compatible logic, we shouldn't 
> > > check HasConstInit.
> > > 
> > > That said, I'm not sure how precisely/in what modes we want to precisely 
> > > emulate MSVC.  Probably anything we do here will be confusing.
> > We should at least issue a warning if the sensible logic and the 
> > MSVC-compatible calculation differ. @rnk, do you know how important it is 
> > to follow the MSVC semantics in this regard?
> I think it depends on whether you think that users are primarily using these 
> pragmas to override the default rdata/bss/data sections without any care 
> about precisely what goes where, or if they are using them to do something 
> finer grained.
> 
> If I had to guess, I'd say it's more likely the former, given that 
> `__declspec(allocate)` and `#pragma(section)` exist to handle cases where 
> users are putting specific globals into specific sections.
> 
> Which, if we follow Richard's suggestion, would mean warning when we put a 
> global marked `const` into a writable section when `ConstSegStack` is 
> non-empty. That seems reasonable. `-Wmicrosoft-const-seg` for the new warning 
> group?
Does the MSVC situation only apply to custom sections? (presumably when not 
customizing the section, MSVC gets it right and can support a const global with 
a runtime initializer, mutable member, or mutating dtor?)

I think this code still needs to be modified, since this is the code that 
drives the error about incompatible sections. So it'll need to behave 
differently depending on the target platform?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156726/new/

https://reviews.llvm.org/D156726

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

aaron.ballman wrote:
> yronglin wrote:
> > dblaikie wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > dblaikie wrote:
> > > > > > Could/should we add some error checking in the ctor to assert that 
> > > > > > we don't overflow these longer values/just hit the bug later on?
> > > > > > 
> > > > > > (& could we use `unsigned short` here rather than bitfields?)
> > > > > We've already got them packed in with other bit-fields from the 
> > > > > expression bits, so I think it's reasonable to continue the pattern 
> > > > > of using bit-fields (that way we don't accidentally end up with 
> > > > > padding between the unnamed bits at the start and the named bits in 
> > > > > this object).
> > > > > 
> > > > > I think adding some assertions would not be a bad idea as a follow-up.
> > > > Maybe some unconditional (rather than only in asserts builds) error 
> > > > handling? (report_fatal_error, if this is low priority enough to not 
> > > > have an elegant failure mode, but something where we don't just 
> > > > overflow and carry on would be good... )
> > > Ping on this? I worry this code has just punted the same bug further 
> > > down, but not plugged the hole/ensured we don't overflow on novel/larger 
> > > inputs.
> > Sorry for the late reply, I was looking through the emails and found this. 
> > I agree add some assertions to check the value is a good idea, It's easy to 
> > help people catch bugs, at least with when `-DLLVM_ENABLE_ASSERTIONS=ON`, 
> > and I'm glad to work on it, but one thing that worries me is that, in 
> > ASTReader, we access this field directly, not through the constructor or 
> > accessor, and we have to add assertions everywhere. 
> > https://github.com/llvm/llvm-project/blob/05b4310c8aec7a050574277ced08a0ab86b27681/clang/lib/Serialization/ASTReaderStmt.cpp#L1382
> I don't think we have to add too many assertions. As best I can tell, we'll 
> need one in each of the `PseudoObjectExpr` constructors and one in 
> `ASTStmtReader::VisitPseudoObjectExpr()`, but those are the only places we 
> assign a value into the bit-field. Three assertions isn't a lot, but if we're 
> worried, we could add a setter method that does the assertion and use the 
> setter in all three places.
My concern wasn't (well, wasn't entirely) about adding more assertions - but 
about having a reliable error here. The patch only makes the sizes larger, but 
doesn't have a hard-stop in case those sizes are exceeded again (which, 
admittedly, is much harder to do - maybe it's totally unreachable now, for all 
practical purposes?) 

I suspect with more carefully constructed recursive inputs could still reach 
the higher limit & I think it'd be good to fail hard in that case in some way? 
(it's probably rare enough that a report_fatal_error would be 
not-the-worst-thing-ever)

But good assertions would be nice too (the old code only failed when you hit 
/exactly/ on just the overflow value, and any more than that the wraparound 
would not crash/fail, but misbehave) - I did add the necessary assertion to 
ArrayRef (begin <= end) which would've helped detect this more reliably, but 
some assert checking for overflow in the ctor would be good too (with all the 
usual nuance/care in checking for overflow) - unless we're going to make that 
into a fatal or other real error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154784/new/

https://reviews.llvm.org/D154784

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
Herald added subscribers: kerbowa, jvesely.
Herald added a project: All.
dblaikie requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Turned out we were making overly simple assumptions about which sections (& 
section flags) would be used when emitting a global into a custom section. This 
lead to sections with read-only flags being used for globals of struct types 
with mutable members.

Fixed by porting the codegen function with the more nuanced handling/checking 
for mutable members out of codegen for use in the sema code that does this 
initial checking/mapping to section flags.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156726

Files:
  clang/include/clang/AST/Type.h
  clang/lib/AST/Type.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/Targets/AMDGPU.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/sections.cpp
  clang/test/SemaCXX/attr-section.cpp

Index: clang/test/SemaCXX/attr-section.cpp
===
--- clang/test/SemaCXX/attr-section.cpp
+++ clang/test/SemaCXX/attr-section.cpp
@@ -48,3 +48,24 @@
 const int const_global_var __attribute__((section("template_fn1"))) = 42;   // expected-error {{'const_global_var' causes a section type conflict with 'template_fn1'}}
 int mut_global_var __attribute__((section("template_fn2"))) = 42;   // expected-note {{declared here}}
 template  __attribute__((section("template_fn2"))) void template_fn2() {} // expected-error {{'template_fn2' causes a section type conflict with 'mut_global_var'}}
+
+namespace mutable_member {
+struct t1 {
+  mutable int i;
+};
+extern const t1 v1;
+__attribute__((section("mutable_member"))) const t1 v1{};
+extern int i;
+__attribute__((section("mutable_member"))) int i{};
+} // namespace mutable_member
+
+namespace non_trivial_ctor {
+struct t1 {
+  t1();
+  constexpr t1(int) { }
+};
+extern const t1 v1;
+__attribute__((section("non_trivial_ctor"))) const t1 v1; // expected-note {{declared here}}
+extern const t1 v2;
+__attribute__((section("non_trivial_ctor"))) const t1 v2{3}; // expected-error {{'v2' causes a section type conflict with 'v1'}}
+} // namespace non_trivial_ctor
Index: clang/test/CodeGenCXX/sections.cpp
===
--- clang/test/CodeGenCXX/sections.cpp
+++ clang/test/CodeGenCXX/sections.cpp
@@ -74,6 +74,14 @@
 #pragma section("short_section", short)
 // Pragma section ignores "short".
 __declspec(allocate("short_section")) short short_var = 42;
+
+struct t1 { mutable int i; };
+extern const t1 mutable_var;
+__declspec(allocate("mutable_section")) const t1 mutable_var;
+
+struct t2 { t2(); };
+extern const t2 non_trivial_ctor;
+__declspec(allocate("non_trivial_ctor_section")) const t2 non_trivial_ctor_var;
 }
 
 //CHECK: @D = dso_local global i32 1
@@ -96,6 +104,8 @@
 //CHECK: @implicitly_read_write = dso_local global i32 42, section "no_section_attributes"
 //CHECK: @long_var = dso_local global i32 42, section "long_section"
 //CHECK: @short_var = dso_local global i16 42, section "short_section"
+//CHECK: @mutable_var = dso_local global %struct.t1 zeroinitializer, section "mutable_section"
+//CHECK: @non_trivial_ctor_var = internal global %struct.t2 zeroinitializer, section "non_trivial_ctor_section"
 //CHECK: define dso_local void @g()
 //CHECK: define dso_local void @h() {{.*}} section ".my_code"
 //CHECK: define dso_local void @h2() {{.*}} section ".my_code"
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -14251,19 +14251,12 @@
   !inTemplateInstantiation()) {
 PragmaStack *Stack = nullptr;
 int SectionFlags = ASTContext::PSF_Read;
-if (var->getType().isConstQualified()) {
-  if (HasConstInit)
-Stack = 
-  else {
-Stack = 
-SectionFlags |= ASTContext::PSF_Write;
-  }
-} else if (var->hasInit() && HasConstInit) {
-  Stack = 
-  SectionFlags |= ASTContext::PSF_Write;
+if (var->hasInit() && HasConstInit &&
+var->getType().isConstantStorage(Context, true, false)) {
+  Stack = 
 } else {
-  Stack = 
   SectionFlags |= ASTContext::PSF_Write;
+  Stack = var->hasInit() && HasConstInit ?  : 
 }
 if (const SectionAttr *SA = var->getAttr()) {
   if (SA->getSyntax() == AttributeCommonInfo::AS_Declspec)
Index: clang/lib/CodeGen/Targets/AMDGPU.cpp
===
--- clang/lib/CodeGen/Targets/AMDGPU.cpp
+++ clang/lib/CodeGen/Targets/AMDGPU.cpp
@@ -419,7 +419,7 @@
 return AddrSpace;
 
   // Only promote to address 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a subscriber: aprantl.
dblaikie added a comment.

In D143967#4441333 , @eddyz87 wrote:

> In D143967#4438815 , @dblaikie 
> wrote:
>
>> I haven't looked closely at this, but from a vague/quick reading, it sounds 
>> like this is creating annotations on certain type DINodes, but then moving 
>> them around to different types? It'd be nice if we could avoid that/create 
>> them in the right place in the first place.
>
> Hi @dblaikie, thank you for taking a look!
>
> I assume you refer to the "placeholder" nodes created for annotated types.
> ...

Yeah, seems unfortunate to break the abstractions and have to add members and 
finalizing for such a narrow feature too. Though I appreciate the complications 
with adding this during type building too.

So you get some bunch of annotations from the `BTFTagAttributedType`, then you 
build the underlying type (which might already be built/needed/fine because 
it's used without attributes in other places/needs to exist independently) - 
and then at the end you copy any of these types that are needed and put the 
annotations on them? Does the `BTFTagAttributedType` always have to apply to 
the immediate type that's going to be modified/mutated to have the attributes 
applied to it directly? Is the set of types that may have these 
attributes/annotations added small/closed? (struct types, void, anything else? 
could you add tags to `int __tag *`, for instance and get a `DW_TAG_base_type` 
for "int" with annotations on it?

If it's really generic/can apply to any type - but always the /immediate/ type 
(I guess with the special handling you've got to skip applying it to the 
`DW_TAG_const_type`, etc)...

What if you skipped `getOrCreateType` - and called into the `CreateType` 
dispatch below that? Since you never want a cached instance of a type, right? 
You want to create a new copy of the type you could then apply annotations to.

Except, I guess, in the instance you showed, where the type is being completed 
- can't go and create another one, because we're part-way through building this 
one... hmm, maybe you can, though? What happens if we start making a totally 
distinct copy of that same type? I guess there's some map that contains such 
partially completed types, and that map would get confused/break if we tried to 
build two types for the same type without adding in some extra key goo that 
contained the annotations... - maybe that wouldn't be too invasive, though?

Hmm - I guess what to put in the type cache would be an interesting question. 
Though that does raise other questions.

given this code:

  #define __tag1 __attribute__((btf_type_tag("tag1")))
  
  struct st {
struct st *self;
  };
  struct st __tag1 x;

What's the expected DWARF here? `x`'s type is a the attributed/annotated `st` 
type, but then does the `self` pointer inside there point to the attributed 
type or the unattributed type?

Maybe what you've got is the best of some bad options, but I'm not sure/trying 
to think it through. (@aprantl wouldn't mind your perspective - if it's too 
much to consume easily, let me know and I can make a best-effort at 
summarizing, maybe even better over a video chat if you've got a few minutes)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

dblaikie wrote:
> aaron.ballman wrote:
> > dblaikie wrote:
> > > Could/should we add some error checking in the ctor to assert that we 
> > > don't overflow these longer values/just hit the bug later on?
> > > 
> > > (& could we use `unsigned short` here rather than bitfields?)
> > We've already got them packed in with other bit-fields from the expression 
> > bits, so I think it's reasonable to continue the pattern of using 
> > bit-fields (that way we don't accidentally end up with padding between the 
> > unnamed bits at the start and the named bits in this object).
> > 
> > I think adding some assertions would not be a bad idea as a follow-up.
> Maybe some unconditional (rather than only in asserts builds) error handling? 
> (report_fatal_error, if this is low priority enough to not have an elegant 
> failure mode, but something where we don't just overflow and carry on would 
> be good... )
Ping on this? I worry this code has just punted the same bug further down, but 
not plugged the hole/ensured we don't overflow on novel/larger inputs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154784/new/

https://reviews.llvm.org/D154784

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

[PATCH] D156546: [Clang][WIP]Experimental implementation of data member packs declarations

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3279
+  assert(NumArgumentsInExpansion && "should not see unknown template 
argument here");
+  for (unsigned Arg = 0; Arg < NumArgumentsInExpansion; ++Arg) {
+// Generate a new field from PackExpansion field.

Not sure if this is a niche opinion, but the `std::optional` operator overloads 
make me a bit uncomfortabel/bit unclear what's happening, and I'd personally 
put a deref in here to make it clearer that this code is assuming (in addition 
to the assert above) the optional is set/present.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3289-3290
+  Fields.push_back(PackedField);
+  if (NewMember->isInvalidDecl())
+Instantiation->setInvalidDecl();
+} else {

Is this codepath tested?



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3292-3295
+  // FIXME: Eventually, a NULL return will mean that one of the
+  // instantiations was a semantic disaster, and we'll want to mark
+  // the declaration invalid. For now, we expect to skip some members
+  // that we can't yet handle.

Worth having a test case showing that/what's broken here?



Comment at: clang/lib/Sema/SemaType.cpp:5930-5931
   break;
+case DeclaratorContext::Member:
+  // Expand for packed data members.
 case DeclaratorContext::TemplateParam:

Perhaps a few more words here would be good - quoting a WG21 proposal paper 
that this code implements, similar to the standard quotations in nearby code. 
(be good to mention the WG21 proposal paper/whatnot in the patch 
description/commit message too)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156546/new/

https://reviews.llvm.org/D156546

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

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

From the other review:

>> Could/should we do the lookup on the CU filename before it goes into the DI 
>> metadata, and store that FileID somewhere for later use?
>
> There's a note in issue 63955 to the effect that I can't find an API to turn 
> a filename into a FileID. If there is one that I didn't find, we could use 
> FileIDs instead of pointers to name strings.

I was thinking more along the lines of keeping the same code we have, string 
pointer equality (though perhaps FileID is equivalent in cases where the 
pointer euqality is valid, and the FileID approach would be more self 
documenting/less subtle). I was thinking - for other files we get filenames 
correctly that are pointer identical because they're in the FileID, right? But 
for this one because we stash it in/get it from the DICompileUnit, that 
identity gets lost. But presumably the identity is true for the string filename 
used to build the DICompileUnit? So if, at that point, we stored the pointer, 
or FileID aside and used that instead of retrieving it from the DICompileUnit, 
perhaps that'd make this name consistent with the other names in being able to 
use pointer identity?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156571/new/

https://reviews.llvm.org/D156571

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

[PATCH] D156546: [Clang][WIP]Experimental implementation of packed data members declarations

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

(just a side note about the title of this patch: I got "packed data members" 
confused and thought this was referring to struct packing 
`__attribute__((packed))` - so perhaps something more like "data member packs" 
would be a more clear term here?)




Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3281-3282
+// Generate a new field from PackExpansion field.
+Decl *NewMember = Instantiator.Visit(Member);
+if (NewMember) {
+  FieldDecl *PackedField = dyn_cast(NewMember);





Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3283
+if (NewMember) {
+  FieldDecl *PackedField = dyn_cast(NewMember);
+  Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(*this, Arg);





Comment at: clang/test/CodeGenCXX/packed_data_member.cpp:8-12
+template struct S2 {
+T t[2];
+Ts... ts;
+};
+

Did this test case come out of any particular bug discovered during 
implementation?



Comment at: clang/test/CodeGenCXX/packed_data_member.cpp:14
+// CHECK: %struct.S1 = type { i32 }
+S1 s1;
+// CHECK-NEXT: %struct.S1.0 = type { i32, float, double }

Not sure, but might be worth a test with multiple of the same type in the pack?



Comment at: clang/test/CodeGenCXX/packed_data_member.cpp:27
+S1<> s5;
\ No newline at end of file


(please add the missing newline at the end of the file here)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156546/new/

https://reviews.llvm.org/D156546

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

[PATCH] D156363: [Driver] -###: exit with code 1 if hasErrorOccurred

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

FWIW this sounds good to me (though given how wide the patch is, might be worth 
waiting a few days to a week in case anyone else has thoughts).

I only looked at a sample of the test changes - if there's anything much more 
interesting than adding `not` to any tests, might be worth calling those out in 
the description/review/etc - and maybe discuss what to do with `not`s that are 
added but because the test is buggy/mistaken. Them being explicit may confuse 
future users into thinking they're intentional, rather than pragmatic. Don't 
really know what to do about that - probably nothing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156363/new/

https://reviews.llvm.org/D156363

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

aaron.ballman wrote:
> dblaikie wrote:
> > Could/should we add some error checking in the ctor to assert that we don't 
> > overflow these longer values/just hit the bug later on?
> > 
> > (& could we use `unsigned short` here rather than bitfields?)
> We've already got them packed in with other bit-fields from the expression 
> bits, so I think it's reasonable to continue the pattern of using bit-fields 
> (that way we don't accidentally end up with padding between the unnamed bits 
> at the start and the named bits in this object).
> 
> I think adding some assertions would not be a bad idea as a follow-up.
Maybe some unconditional (rather than only in asserts builds) error handling? 
(report_fatal_error, if this is low priority enough to not have an elegant 
failure mode, but something where we don't just overflow and carry on would be 
good... )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154784/new/

https://reviews.llvm.org/D154784

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

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:596-597
 
-// These don't need to be particularly wide, because they're
-// strictly limited by the forms of expressions we permit.
-unsigned NumSubExprs : 8;
-unsigned ResultIndex : 32 - 8 - NumExprBits;
+unsigned NumSubExprs : 16;
+unsigned ResultIndex : 16;
   };

Could/should we add some error checking in the ctor to assert that we don't 
overflow these longer values/just hit the bug later on?

(& could we use `unsigned short` here rather than bitfields?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154784/new/

https://reviews.llvm.org/D154784

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

[PATCH] D154366: [clang][ExprConstant] Print template arguments when describing stack frame

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@hazohelet can you commit this yourself, or do you need someone to commit it on 
your behalf? (& if so, what name/email address would you like to be credited)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154366/new/

https://reviews.llvm.org/D154366

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

[PATCH] D156143: Add Adrian and David as owners for debug info

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Sounds good, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156143/new/

https://reviews.llvm.org/D156143

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

[PATCH] D155991: [DWARF] Make sure file entry for artificial functions has an MD5 checksum

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Any memory usage measurements to check this doesn't have a significant adverse 
impact by copying all the strings? (or performance by having to do string 
rather than pointer equality comparisons?)

Could/should we do the lookup on the CU filename before it goes into the DI 
metadata, and store that FileID somewhere for later use? Rather than requerying 
it later/making all queries string comparisons/storing strings, etc? (we could 
put an "expensive checks" assertion in that our queries always return in 
agreement with the pointer equality - so in case there are other things that 
trip over this issue we'll know about them sooner)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155991/new/

https://reviews.llvm.org/D155991

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D153536#4521156 , 
@hubert.reinterpretcast wrote:

> In D153536#4512897 , @dblaikie 
> wrote:
>
>> but at least at a first blush I can't reproduce the failures shown...
>
> @dblaikie, you //did// reproduce the issue with the members. Both entries 
> have DW_AT_decl_line 2 and DW_AT_data_member_location 0 (the second entry 
> should indicate DW_AT_decl_line 3 and DW_AT_data_member_location 4):
>
>>   0x002e:   DW_TAG_structure_type
>>   DW_AT_calling_convention(DW_CC_pass_by_value)
>>   DW_AT_name  ("t1")
>>   DW_AT_byte_size (0x08)
>>   DW_AT_decl_file 
>> ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
>>   DW_AT_decl_line (1)
>>   
>>   0x0034: DW_TAG_member
>> DW_AT_name("_")
>> DW_AT_type(0x0047 "int")
>> DW_AT_decl_file   
>> ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
>> DW_AT_decl_line   (2)
>> DW_AT_data_member_location(0x00)
>>   
>>   0x003d: DW_TAG_member
>> DW_AT_name("_")
>> DW_AT_type(0x0047 "int")
>> DW_AT_decl_file   
>> ("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
>> DW_AT_decl_line   (2)
>> DW_AT_data_member_location(0x00)
>>   
>>   0x0046: NULL
>
> As for the block-scope case, I am still able to reproduce the issue (and also 
> your result that does not exhibit the issue). The key seems to be having the 
> `_`s on the same line.

Ah, fair enough - probably easy enough for you to look for some kind of map 
with inadequate key material in CGDebugInfo somewhere, I'd guess...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153536/new/

https://reviews.llvm.org/D153536

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> I think getting new system headers is harder for projects to deal with, so I 
> think I'm now in agreement with you that we should enable the warning as an 
> error in system headers.

Awesome - glad we're on the same page :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D150226#4515843 , @aaron.ballman 
wrote:

> In D150226#4515783 , @dblaikie 
> wrote:
>
>> In D150226#4498024 , 
>> @aaron.ballman wrote:
>>
>>> In D150226#4461846 , @efriedma 
>>> wrote:
>>>
 The fundamental problem here is the interaction with SFINAE; if we don't 
 diagnose this as an error, we actually miscompile some cases.  Because of 
 that, a flag wouldn't really be "turning off the warning"; it would be 
 enabling a non-compliant language mode that has different rules for 
 whether enum casts are legal.

 If it weren't for that, I don't think anyone would be in a hurry to turn 
 the warning into a hard error.
>>>
>>> +1 to this. I'm worried about the miscompiles continuing in the wild. We 
>>> did the best we could when landing the warning-as-error changes in D131307 
>>>  by telling users explicitly "This 
>>> diagnostic is expected to turn into an error-only diagnostic in the next 
>>> Clang release." in the release notes. Obviously, that's not ideal because 
>>> very few folks read release notes, but we don't have any better mechanism 
>>> for communicating these kinds of changes.
>>>
>>> What do folks think is a reasonable path forward here? Given that we're 
>>> going to branch the Clang 17 release in a few weeks (AIUI), my suggestion 
>>> is to kick the can down the road by landing these changes just after we 
>>> branch. That way the changes land such that early adopters can test and see 
>>> how disruptive we're being without time pressure or hassle of dealing with 
>>> release branches. Do folks think that's a reasonable approach? (I'm open to 
>>> other suggestions.)
>>
>> I'm not quite following if/what the objection is to removing the "ignore in 
>> system headers" for the warning for a release or two prior to making this a 
>> hard error? That seems like a fairly low-cost change (it does kick the can 
>> down the road a release or two before it becomes a hard error - but isn't 
>> worse for users, for the most part - if they were going to get a hard error 
>> from a system header before, at least now instead they'd get a warning or 
>> warning-defaulted-to-error instead, they could turn it off (but they had 
>> been warned that their system headers were going to cause them problems 
>> soon) and then it becomes a hard error a release or two later).
>
> Thank you for bringing that up, sorry for not being more explicit -- by 
> "these changes", I meant "whatever form we decide they should take" as 
> opposed to "the patch as it is now."

Ah, OK.

> In terms of removing the "ignore in system headers" flag, I'm not strongly 
> opposed to it, but I do worry it will throw the baby out with the bathwater. 
> The specific use case I'm worried about is: user has a system header with the 
> problematic code, they run into this diagnostic and they can't address it 
> themselves so they disable the warning for the project. The user then doesn't 
> learn about the problems in their own (non-system header) code until it 
> becomes a hard error later.

OK, I think we're still talking past each other/making different comparisons.
I think you're comparing "make this a warning in system headers" to "not doing 
anything" (so, yes, it causes people to ignore the warning even in their own 
code when if they had done nothing/we had done nothing to force them, they 
would've kept getting the warning in their code)
Whereas I'm comparing "make this a warning in system headers" to "making this a 
hard error" (in which case a user for which it would've been a hard error and 
they'd have had no choice but to fix their system headers (might be 
hard/impossible) would have some relief valve for a time/some notice that this 
would be a problem in the future when it becomes a hard error)

If we picture a timeline:
Time 1) Code is valid/no problem
Time 2) Warning added (not in system headers)
Time 3) Warning becomes error by default (but could be disabled by a user) 
(still not in system headers)
Time 4) Becomes hard error

And if someone has a system header with a violation, they won't know about it 
until (4) and they won't be able to do much/anything about it.

I'm suggesting making the process longer.

Time 4) default-error warning becomes enabled in system headers

Users who, in the first timeline, would be stuck - at least have a release 
valve. I think they are better off than they were in the first timeline - even 
though they lose warning coverage over their own code (which is bad), at least 
they can still build their project and know they have problems in their system 
headers they should report/etc and try to have addressed.

then Time 5) make it a hard error.

So it takes a bit longer to get to (5), and users who would be OK in the 
original 

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D150226#4498024 , @aaron.ballman 
wrote:

> In D150226#4461846 , @efriedma 
> wrote:
>
>> The fundamental problem here is the interaction with SFINAE; if we don't 
>> diagnose this as an error, we actually miscompile some cases.  Because of 
>> that, a flag wouldn't really be "turning off the warning"; it would be 
>> enabling a non-compliant language mode that has different rules for whether 
>> enum casts are legal.
>>
>> If it weren't for that, I don't think anyone would be in a hurry to turn the 
>> warning into a hard error.
>
> +1 to this. I'm worried about the miscompiles continuing in the wild. We did 
> the best we could when landing the warning-as-error changes in D131307 
>  by telling users explicitly "This 
> diagnostic is expected to turn into an error-only diagnostic in the next 
> Clang release." in the release notes. Obviously, that's not ideal because 
> very few folks read release notes, but we don't have any better mechanism for 
> communicating these kinds of changes.
>
> What do folks think is a reasonable path forward here? Given that we're going 
> to branch the Clang 17 release in a few weeks (AIUI), my suggestion is to 
> kick the can down the road by landing these changes just after we branch. 
> That way the changes land such that early adopters can test and see how 
> disruptive we're being without time pressure or hassle of dealing with 
> release branches. Do folks think that's a reasonable approach? (I'm open to 
> other suggestions.)

I'm not quite following if/what the objection is to removing the "ignore in 
system headers" for the warning for a release or two prior to making this a 
hard error? That seems like a fairly low-cost change (it does kick the can down 
the road a release or two before it becomes a hard error - but isn't worse for 
users, for the most part - if they were going to get a hard error from a system 
header before, at least now instead they'd get a warning or 
warning-defaulted-to-error instead, they could turn it off (but they had been 
warned that their system headers were going to cause them problems soon) and 
then it becomes a hard error a release or two later).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

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

[PATCH] D134544: [clang-cl] Implement /ZH: flag

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/Driver/Options.td:3220
   Values<"simple,mangled">, Flags<[CC1Option, NoDriverOption]>;
+def gsrc_hash_EQ : Joined<["-"], "gsrc-hash=">,
+  Group, Flags<[CC1Option, NoDriverOption]>,

thakis wrote:
> dblaikie wrote:
> > This is a driver flag, yeah? Might be worth documenting/emitting a warning 
> > that it's not compatible with DWARF emission? (looks like the DWARF code in 
> > LLVM ignores any hash that's not MD5, because DWARF doesn't have a way to 
> > encode other hash algorithms currently - though maybe there's some 
> > extension space available to add it, I haven't checked/looked)
> belated: this is `Flags<[CC1Option, NoDriverOption]>,` in the line below, so 
> I think this isn't a driver flag. (It's what the CLFlags added below expand 
> to – those are driver flags, but only for clang-cl.)
Oh, I see - I think I was confused by the driver code referencing 
`gsrc_hash_EQ` - but that's because the `/ZH` are implemented as aliases for 
`gsrc_hash_EQ`.

I guess then maybe my comment applies to `/ZH` - if someone asks for DWARF and 
uses that flag? But maybe not worth it... 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134544/new/

https://reviews.llvm.org/D134544

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D153536#4490918 , @cor3ntin wrote:

> @dblaikie Would you be willing to look at the debugger side of things in a 
> subsequent patch? I'm not familiar with debug symbol code gen so I'm not sure 
> I'd be able to improve thing the right way.

Not sure I've got the time to do the fix myself, but might be able to provide 
pointers.

but at least at a first blush I can't reproduce the failures shown...

  struct t1 {
int _;
int _;
  };
  t1 v1;
  int main() {
int _;
int _;
  }

  0x002e:   DW_TAG_structure_type
  DW_AT_calling_convention(DW_CC_pass_by_value)
  DW_AT_name  ("t1")
  DW_AT_byte_size (0x08)
  DW_AT_decl_file 
("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
  DW_AT_decl_line (1)
  
  0x0034: DW_TAG_member
DW_AT_name("_")
DW_AT_type(0x0047 "int")
DW_AT_decl_file   
("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
DW_AT_decl_line   (2)
DW_AT_data_member_location(0x00)
  
  0x003d: DW_TAG_member
DW_AT_name("_")
DW_AT_type(0x0047 "int")
DW_AT_decl_file   
("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
DW_AT_decl_line   (2)
DW_AT_data_member_location(0x00)
  
  0x0046: NULL
  
  0x0047:   DW_TAG_base_type
  DW_AT_name  ("int")
  DW_AT_encoding  (DW_ATE_signed)
  DW_AT_byte_size (0x04)
  
  0x004b:   DW_TAG_subprogram
  DW_AT_low_pc(0x)
  DW_AT_high_pc   (0x0008)
  DW_AT_frame_base(DW_OP_reg6 RBP)
  DW_AT_name  ("main")
  DW_AT_decl_file 
("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
  DW_AT_decl_line (6)
  DW_AT_type  (0x0047 "int")
  DW_AT_external  (true)
  
  0x005a: DW_TAG_variable
DW_AT_location(DW_OP_fbreg -4)
DW_AT_name("_")
DW_AT_decl_file   
("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
DW_AT_decl_line   (7)
DW_AT_type(0x0047 "int")
  
  0x0065: DW_TAG_variable
DW_AT_location(DW_OP_fbreg -8)
DW_AT_name("_")
DW_AT_decl_file   
("/usr/local/google/home/blaikie/dev/scratch/unused_member.cpp")
DW_AT_decl_line   (8)
DW_AT_type(0x0047 "int")

Looks OK to me - two local variables with the same name, two member variables 
with the same name.

so probably at least one bug in lldb because it does seem to think `t1` has 
only one member. But the DWARF I see for the local variables doesn't seem to 
match the dump shown in https://reviews.llvm.org/D153536#4483191


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153536/new/

https://reviews.llvm.org/D153536

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

[PATCH] D154366: [clang][ExprConstant] Print template arguments when describing stack frame

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds good to me


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154366/new/

https://reviews.llvm.org/D154366

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D153536#4475308 , @cor3ntin wrote:

> In D153536#4475240 , @dblaikie 
> wrote:
>
>> In D153536#4475199 , @cor3ntin 
>> wrote:
>>
>>> In D153536#4474733 , @dblaikie 
>>> wrote:
>>>
 Maybe try testing with more different values, to demonstrate which 
 entities the debugger is finding?

   int _ = 1;
   int _ = 2;
   {
 int _ = 3;
 int _ = 7;
   }

 Or something like that. But, honestly, if the point is that these 
 variables should be unnameable - perhaps we shouldn't generate any DWARF 
 for them?
>>>
>>> The variables should still be inspectable, ideally. It's true it makes no 
>>> sense to be able to use them in expressions, and maybe if lldb use clang to 
>>> evaluate expressions that would just work?
>>
>> I think that's going to be tricky - because we don't emit DWARF to say where 
>> one variable's lifetime starts compared to another in the same scope - so 
>> likely a debugger would just always show the first or last variable with the 
>> same name in a given scope (so in the above example you could probably only 
>> print {1,3} or {2,7}) - and getting it wrong is more likely and more 
>> problematic than existing cases of shadowed variables. If we can't get this 
>> right, which I don't think we can, practically speaking, at the moment (the 
>> DWARF feature we'd have to support to do this better would probably be 
>> `DW_AT_start_scope` to say at what instruction the lifetime of a variable 
>> starts) - it may be best not to include it at all?
>
> It doesn't seems to be confused on simple examples
>
>   cpp
>   * thread #1, name = 'placeholder', stop reason = step in
>   frame #0: 0x5157 placeholder`main at 
> debug_placeholder.cpp:8:13
>  5{
>  6int _ = 4;
>  7int _ = 5;
>   -> 8int _ = 6;
>  9}
>  10   }
>   (lldb) frame variable
>   (int) _ = 1
>   (int) _ = 2
>   (int) _ = 3
>   (int) _ = 4
>   (int) _ = 5
>   (int) _ = -9432
>
> That being said, if you think it's more prudent, I would not mind

Huh, I guess maybe it's got some heuristics based on line number. Could 
probably thwart those by stamping this out with a macro (which would cause all 
code in the macro to be attributed to the line where the macro is named) - add 
some function calls in, so you can step back up from the call to get to more 
precise locations and you'd probably see some problems. That's probably a bit 
contrived (but maybe not - perhaps `_` would be more common in macros).

*shrug* maybe it's OK enough.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153536/new/

https://reviews.llvm.org/D153536

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D153536#4475199 , @cor3ntin wrote:

> In D153536#4474733 , @dblaikie 
> wrote:
>
>> Maybe try testing with more different values, to demonstrate which entities 
>> the debugger is finding?
>>
>>   int _ = 1;
>>   int _ = 2;
>>   {
>> int _ = 3;
>> int _ = 7;
>>   }
>>
>> Or something like that. But, honestly, if the point is that these variables 
>> should be unnameable - perhaps we shouldn't generate any DWARF for them?
>
> The variables should still be inspectable, ideally. It's true it makes no 
> sense to be able to use them in expressions, and maybe if lldb use clang to 
> evaluate expressions that would just work?

I think that's going to be tricky - because we don't emit DWARF to say where 
one variable's lifetime starts compared to another in the same scope - so 
likely a debugger would just always show the first or last variable with the 
same name in a given scope (so in the above example you could probably only 
print {1,3} or {2,7}) - and getting it wrong is more likely and more 
problematic than existing cases of shadowed variables. If we can't get this 
right, which I don't think we can, practically speaking, at the moment (the 
DWARF feature we'd have to support to do this better would probably be 
`DW_AT_start_scope` to say at what instruction the lifetime of a variable 
starts) - it may be best not to include it at all?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153536/new/

https://reviews.llvm.org/D153536

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

[PATCH] D154366: [clang][ExprConstant] Print template arguments when describing stack frame

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Do we not already have some function name printing helper we could be reusing 
that might have some smarts about not printing deduced template arguments, 
using preferred names, whatever else?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154366/new/

https://reviews.llvm.org/D154366

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

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Maybe try testing with more different values, to demonstrate which entities the 
debugger is finding?

  int _ = 1;
  int _ = 2;
  {
int _ = 3;
int _ = 7;
  }

Or something like that. But, honestly, if the point is that these variables 
should be unnameable - perhaps we shouldn't generate any DWARF for them?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153536/new/

https://reviews.llvm.org/D153536

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

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D152017#4462263 , @probinson wrote:

>> we might emit member function declarations for call sites in DWARF, for 
>> instance
>
> So are you now leaning more toward wanting to emit the "used" declarations as 
> well? I'm sure you don't want to implement it the way we did...

There's already infrastructure to do this for non-member functions, and if we 
do it the way we do for things like member function template instantiations 
(they don't go in the "members" list, they just list the type as their 
"scope"/parent) - it should be relatively easy/the same as for non-member 
functions, I think.

> I *think* the template parameter thing has more to do with not supporting 
> expressions in all their glory, although I'm fuzzy on that--last time I 
> looked at that stuff was a number of years ago.

Yeah, there are some cases that are really hairy with whole expression 
mangling, which is unfortunate and I'm not sure what we could do there short of 
embed a string with the expression in it... :/ but there are some more cases we 
could do better at with more semantic information than that, such as the case I 
mentioned like a non-type template parameter of pointer type pointing to a 
member function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152017/new/

https://reviews.llvm.org/D152017

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

[PATCH] D153898: [DebugInfo] Enable debug info emission for extern variables in C++

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.

In D153898#4457263 , @chiyuze wrote:

> Regarding cost, only BPF target triggers this debug info generation for 
> extern variables.

Ah, OK - if that's what you need for BPF and it doesn't affect anything else - 
have at.

(though I'd still ask a bit about the issue of declaration V definition - is 
BPF just always "standalone" debug info generally? (is there a problem with the 
definition coming from another translation unit with debug info?)?)

> There is no impact on x86 builds of clang:

Oh, OK - no worries then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153898/new/

https://reviews.llvm.org/D153898

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

[PATCH] D153898: [DebugInfo] Enable debug info emission for extern variables in C++

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Got some details on how much this costs? (eg: `bloaty` comparison for a clang 
bootstrap with/without this patch applied?)
Could you provide a quick summary of why this debug info is required? The 
definition should be provided in whatever translation unit defines the variable 
and you should be able to get debug info from there - if that translation unit 
is built without -g, then maybe this feature should be gated on 
`-fstandalone-debug` which is intended to express the intent to get debug info 
coverage when a translation unit may be the only one built with `-g`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153898/new/

https://reviews.llvm.org/D153898

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

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Basing this on "defined" or "undefined" member functions isn't /perfectly/ 
correct (though might be correct enough for this flag - though I do prefer 
something about "canonicality" of types, but perhaps it's too inside-baseball 
for the average user - though these debug info flags are pretty esoteric at the 
best of times anyway) - we might emit member function declarations for call 
sites in DWARF, for instance (seems currently we don't emit call sites for 
member function calls, but I don't think it's a fundamental issue - just 
something no one's implemented yet/maybe a heuristic for what's worth 
describing call sites for) - also, while we don't do it currently, I've just 
been thinking about template info completeness and we don't currently reference 
DIEs for non-type-template parameters of pointer type, including 
function/member function pointers, and it'd be nice if we did point to the 
actual DIE describing the thing we're pointing to, in which case we'd need to 
emit some more function declarations there too.

Thoughts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152017/new/

https://reviews.llvm.org/D152017

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

I haven't looked closely at this, but from a vague/quick reading, it sounds 
like this is creating annotations on certain type DINodes, but then moving them 
around to different types? It'd be nice if we could avoid that/create them in 
the right place in the first place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143967/new/

https://reviews.llvm.org/D143967

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

[PATCH] D153362: [clang][DebugInfo] Emit DW_AT_defaulted for defaulted C++ member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Any particular use for this debug info in DWARF consumers?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153362/new/

https://reviews.llvm.org/D153362

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

[PATCH] D151575: [clang][diagnostics] Always show include stacks on errors

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Judging by @aaron.ballman's comment on the bug, I can see the logic here.

Though perhaps this notion generalizes to all non-note types, not just errors 
(like perhaps we should print the include stack for each warning, too?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151575/new/

https://reviews.llvm.org/D151575

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> My concern with `ShowInSystemHeaders` is that this seems like a bad user 
> experience.

I guess you're comparing this user experience to the one when this feature was 
a normal warning/without `ShowInSystemHeaders` - I'm comparing this situation 
to the future where this becomes a hard error with no escape hatch. 
`ShowInSystemHeaders` is more aggressive than the former & yeah, isn't terribly 
nice for users - but it's less aggressive than the latter, and gives users some 
escape hatch for now until the thing becomes a hard error & at least they know 
it's coming, maybe? (with enough text in the warning, etc)

So, yeah, I'd be inclined to make the change to `ShowInSystemHeaders` close to 
when you were going to make it a hard error, pushing back the hard-error change 
a little bit (maybe just one clang release? Maybe a couple? Not sure).

> If the system header triggers an error 1) some users aren't going to know how 
> to fix that by downgrading the diagnostic, so that may cause them to go 
> "Clang is buggy because  accepts this fine" (not the end of 
> the world, but frustrates both us and users). 2) the only recourse users have 
> is to downgrade/disable the diagnostic (otherwise they'd have to change 
> system header code), which they may likely do with a command line flag rather 
> than something more targeted like diagnostic pragmas around the include, 
> which increases the risk of users not seeing the issues in code they can 
> control.

They'd already have had a chance to deal with their code when this was a 
warning-default-error without "ShowInSystemHeaders"? (or, if the yhaven't 
picked up a new compiler often enough, and they go from "a warning we didn't 
care about" to "warning-default-error-with-ShowInSystemHeaders" - they're still 
better off than if it'd gone straight to hard error, some chance to cleanup 
while disabling the warning/error before picking up a compiler version that 
makes it a hard error)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D150226#4417539 , @aaron.ballman 
wrote:

> In D150226#4408980 , @dblaikie 
> wrote:
>
>> In D150226#4408563 , @erichkeane 
>> wrote:
>>
>>> In D150226#4408381 , 
>>> @aaron.ballman wrote:
>>>
 In D150226#4404808 , @rupprecht 
 wrote:

> I suppose including this warning in `ShowInSystemHeader` with your diff 
> above would be a good first step anyway, right? If we're about to make it 
> a hard error anyway, then making it a `ShowInSystemHeader` error first 
> would ease that transition.

 Yes and no.

 If we're going to turn something into a hard error, letting folks 
 implementing system headers know about it is important, so from that 
 perspective, it makes sense to enable the diagnostic in system headers. 
 However, *users* have no choice in their system headers (oftentimes) which 
 makes the diagnostic unactionable for them as they're not going to (and 
 shouldn't have to) modify system headers, so from that perspective, 
 enabling the diagnostic in a system header introduces friction.

 If this diagnostic is showing up in system headers, I think we would 
 likely need to consider adding a compatibility hack to allow Clang to 
 still consume that system header while erroring when outside of that 
 system header (we've done this before to keep STL implementations working, 
 for example). Between this need and the friction it causes users to have 
 an unactionable diagnostic, I think we probably should not enable 
 `ShowInSystemHeader`.
>>>
>>> It seems to me that if our concern is breaking system headers, we need to 
>>> do that with better testing.  Some sort of 'diagnostic group' for "This is 
>>> going to become an error *SOON*" mixed with us/vendors running that on 
>>> platforms we consider significant enough to not break.  But just diagnosing 
>>> on arbitrary users with no choice on how to fix the headers doesn't seem 
>>> appropriate.
>>
>> I think that's the request here: 
>> https://github.com/llvm/llvm-project/issues/63180
>
> +1, I think that request is a reasonable idea to help maintainers of system 
> headers.
>
> It sounds like we're in agreement that we should not enable 
> `ShowInSystemHeaders` for this, but it's not clear whether we've got 
> agreement yet that we can land this change right now. I think it's acceptable 
> to kick the can down the road by another release or so if we feel we need to, 
> but there is a hard stop to that at some point (some folks won't fix their 
> code until they're forced to do so, and there's not much we can do about 
> those cases).

FWIW, I still think it might be reasonable to `ShowInSystemHeaders` before 
turning something into an unconditional/hard error - `ShowInSystemHeaders` is 
strictly less intrusive than a hard error, and more intrusive than the warning 
as-is, so seems like a reasonable part of transitioning to a feature removal. 
(warning -> default-error -> show in system headers -> hard error) I don't have 
enough of an investment in this area to suggest that this /must/ be the way 
such a transition is done, but I have a hard time seeing why it would be better 
to avoid that intermediate step.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

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

[PATCH] D152708: [RFC][Draft] Enable primitive support for Two-Level Line Tables in LLVM

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Are there any known (or vague/unknown) limitations on the implementation with 
respect to the actual output/on-disk representation? (like, would doing some 
kind of size analysis of the output when using this patch/feature lead to 
incorrect conclusions about the cost of the feature?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152708/new/

https://reviews.llvm.org/D152708

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

[PATCH] D152708: [RFC][Draft] Enable primitive support for Two-Level Line Tables in LLVM

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Really appreciate you providing this prototype.

> This solves one of the current weaknesses in DWARF debugging, which is the 
> inability to set breakpoints or step onto inlined callsites. However, there 
> are other proposals (such as Location View Numbering) that could achieve the 
> same thing, and the costs of TLLT in storage size and design complexity may 
> make it a non-ideal solution to this problem.

FWIW, one of the major motivations for this was not only to include the stop 
points, but also to allow symbolizing including stack frames - so you'd need to 
know that all the inlined instructions come from the inlined call site. I 
think, maybe, my vague understanding of Location View Numbering wouldn't cover 
that case, and only covers the "how do I describe the variable when I'm stopped 
at this synthetic location that exists just before or after the call", for 
instance?

It is unfortunate to hear that TLLT are a significant size increase, though not 
entirely surprising - it's a bunch of extra info to encode. I'll be glad to 
have this example to experiment with.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152708/new/

https://reviews.llvm.org/D152708

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

[PATCH] D143984: [DebugMetadata] Simplify handling subprogram's retainedNodes field. NFCI (1/7)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

When we migrated to github we gained higher fidelity for attributing authors 
(we can now commit on someone's behalf but have the commit author properly 
recorded as the real author - we couldn't do that back on Subversion) - but I 
think it's meant that fail-mail from bots goes to that author, and not to the 
committer. Ideally it'd go to both - so, as it stands now, being a 
commiter-but-not-author is tricky, because you don't get the feedback on the 
commit you made. You might watch buildbots manually, or at least bhe responsive 
to other people complaining on the reviews like this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143984/new/

https://reviews.llvm.org/D143984

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

[PATCH] D152447: [Clang] Remove -no-opaque-pointers cc1 flag

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: t.p.northover, dblaikie.
dblaikie added a comment.

(I hope this doesn't come off as condescending/patronizing, I mean it quite 
genuinely)

A deep thanks to everyone who worked on this, especially @aeubanks and @nikic, 
and @t.p.northover - it's been a long project and I really appreciate you folks 
picking up where I stalled out years ago/paying down the technical debt I 
created (which I'm sorry about).
Starting with 
https://lists.llvm.org/pipermail/llvm-dev/2015-February/081822.html and the 
first change, I think, was D7655 , so... 8 
years, 4 months? Glad it's come along.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152447/new/

https://reviews.llvm.org/D152447

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D150226#4408563 , @erichkeane 
wrote:

> In D150226#4408381 , @aaron.ballman 
> wrote:
>
>> In D150226#4404808 , @rupprecht 
>> wrote:
>>
>>> I suppose including this warning in `ShowInSystemHeader` with your diff 
>>> above would be a good first step anyway, right? If we're about to make it a 
>>> hard error anyway, then making it a `ShowInSystemHeader` error first would 
>>> ease that transition.
>>
>> Yes and no.
>>
>> If we're going to turn something into a hard error, letting folks 
>> implementing system headers know about it is important, so from that 
>> perspective, it makes sense to enable the diagnostic in system headers. 
>> However, *users* have no choice in their system headers (oftentimes) which 
>> makes the diagnostic unactionable for them as they're not going to (and 
>> shouldn't have to) modify system headers, so from that perspective, enabling 
>> the diagnostic in a system header introduces friction.
>>
>> If this diagnostic is showing up in system headers, I think we would likely 
>> need to consider adding a compatibility hack to allow Clang to still consume 
>> that system header while erroring when outside of that system header (we've 
>> done this before to keep STL implementations working, for example). Between 
>> this need and the friction it causes users to have an unactionable 
>> diagnostic, I think we probably should not enable `ShowInSystemHeader`.
>
> It seems to me that if our concern is breaking system headers, we need to do 
> that with better testing.  Some sort of 'diagnostic group' for "This is going 
> to become an error *SOON*" mixed with us/vendors running that on platforms we 
> consider significant enough to not break.  But just diagnosing on arbitrary 
> users with no choice on how to fix the headers doesn't seem appropriate.

I think that's the request here: 
https://github.com/llvm/llvm-project/issues/63180 - some way, initially, to 
opt-in to such a diagnostic group or mechanism (like "enable this warning in 
system headers") to assess how big the migration effort is/report back on 
whether it's necessary to implement a compatibility hack or whether things will 
be able to be cleaned up. (such a thing could be opt-in at first, vendors etc 
could use that to assess the fallout - if the answer is "we think we can clean 
this up" and folks plan some timeline, they reckon they're ready, then you 
switch to that feature being enabled by default (because we think we can make 
it an error - but there's always surprises, so shipping it as an 
error-by-default-but-with-a-way-to-opt-out is better than shipping it as a hard 
error that catches users by surprise) - then if there's no major fallout, 
remove the ability to opt-out)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

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

[PATCH] D124221: Reimplement `__builtin_dump_struct` in Sema.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

https://github.com/llvm/llvm-project/issues/63169


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124221/new/

https://reviews.llvm.org/D124221

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

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D152017#4400735 , @probinson wrote:

> Oh, `-fstandalone-debug` should override this? In the Sony implementation, it 
> does. (Sorry for not remembering that sooner.)

I'd think of this as orthogonal to that, potentially. by analogy with 
non-member functions. Even in standalone debug, we wouldn't emit a declaration 
for every function declared in a translation unit. Ah, because we include the 
mangled names on member function declarations, having that would still allow 
you to call the function from another translation unit, if it's available, even 
without DWARF for that TU, I suppose.

So, while it's inconsistent with non-member functions, which doesn't seem like 
a good principle - yeah, guess we could imply this option is disabled by 
`-fstandalone-debug`...

In D152017#4400449 , @rnk wrote:

> I tend to think that enabled-by-default options which have names that you are 
> supposed to prefix with no are awkward[1], but the compiler ecosystem (GCC & 
> Clang) has lots of those, and perhaps we should lean into it. I like 
> `-g[no-]undefined-methods` the best of Paul's suggestions.
>
> [1]  `-fdelete-null-pointer-checks`, anyone? Yes, that's a feature I want in 
> my compiler, please delete my null checks, I didn't need those, sign me up!

Yeah, mixed feelings about a default-on flag, so you have to use `-gno-*`. 
Though one name in that form, might be `-gno-canonical-types`.
(pedantically C++ calls these things "member functions" (& it seems nice to use 
consistent naming) rather than "methods", but 
`-g[no-]undefined-member-functions` would be a bit of a mouthful... )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152017/new/

https://reviews.llvm.org/D152017

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

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D152017#4399989 , @probinson wrote:

> My debugger guy says "this shouldn't be a problem."
>
> Given that, my request is that `-gincomplete-types` should be default-true 
> for `DebuggerTuning == SCE` if you want to commit this; otherwise I'll redo 
> our downstream patch to match yours.

Happy to do the SCE tuning in a follow-up patch.

Is the name good for you? (not that you'd need to interact with it much if it's 
the default for your target anyway)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152017/new/

https://reviews.llvm.org/D152017

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

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

oh, side note: We don't emit DWARF descriptions of called non-member functions, 
generally (well, more often these days with optimized debug info call_site info 
we do need to emit function declarations for called functions) - so by analogy 
I'd have thought doing similarly for member functions would be suitable


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152017/new/

https://reviews.llvm.org/D152017

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

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D152017#4396906 , @probinson wrote:

> I experimented with this. Looks like it emits info only for //defined// 
> methods, and not //used// methods. That is, if you change the test to say
> `void t1::f1() { f2(); }`
> you get DWARF for f1 but not f2. The way Sony does it, you get DWARF for f1 
> and f2.
> (Neither case sees info for f3, which is declared but not defined or used.)

Ah, that's good to understand - didn't realize that's what you folks were 
doing. Fair enough then.

> Is that what you intended?

It is.

What's the particular goal/value in including called-but-not-defined functions? 
Are your users generally building only parts of their program with debug info & 
you want it to be complete-ish in the parts that do have debug info? Because if 
they were building the whole program with debug info, /some/ translation unit 
would have the definition of the function, and the debug info for it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152017/new/

https://reviews.llvm.org/D152017

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

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 527920.
dblaikie added a comment.

Updating commit message with extra details


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152017/new/

https://reviews.llvm.org/D152017

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGenCXX/debug-info-incomplete-types.cpp
  clang/test/Driver/debug-options.c


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -259,6 +259,9 @@
 // RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
 // RUN: %clang -### -c -fdebug-ranges-base-address 
-fno-debug-ranges-base-address %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
 //
+// RUN: %clang -### -c -gincomplete-types %s 2>&1 | FileCheck 
-check-prefix=INCTYPES %s
+// RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NOINCTYPES %s
+//
 // RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=NOPUB %s
 // RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck 
-check-prefix=NOPUB %s
 //
@@ -398,6 +401,9 @@
 // RNGBSE: -fdebug-ranges-base-address
 // NORNGBSE-NOT: -fdebug-ranges-base-address
 //
+// INCTYPES: -gincomplete-types
+// NOINCTYPES-NOT: -gincomplete-types
+//
 // GARANGE-DAG: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
Index: clang/test/CodeGenCXX/debug-info-incomplete-types.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-incomplete-types.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -gincomplete-types %s -emit-llvm 
-o - | FileCheck %s
+
+struct t1 {
+  void f1();
+  void f2();
+};
+
+void t1::f1() { }
+
+// CHECK: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1"
+// CHECK-SAME: elements: [[ELEMENTS:![0-9]+]]
+// CHECK: [[ELEMENTS]] = !{}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4308,6 +4308,12 @@
  DebuggerTuning != llvm::DebuggerKind::DBX)))
 CmdArgs.push_back("-gno-column-info");
 
+  if (const Arg *A = Args.getLastArg(options::OPT_gincomplete_types))
+(void)checkDebugInfoOption(A, Args, D, TC);
+  if (Args.hasFlag(options::OPT_gincomplete_types,
+   options::OPT_gno_incomplete_types, false))
+CmdArgs.push_back("-gincomplete-types");
+
   // FIXME: Move backend command line options to the module.
   if (Args.hasFlag(options::OPT_gmodules, options::OPT_gno_modules, false)) {
 // If -gline-tables-only or -gline-directives-only is the last option it
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2740,7 +2740,7 @@
 
   // Collect data fields (including static variables and any initializers).
   CollectRecordFields(RD, DefUnit, EltTys, FwdDecl);
-  if (CXXDecl)
+  if (CXXDecl && !CGM.getCodeGenOpts().DebugIncompleteTypes)
 CollectCXXMemberFunctions(CXXDecl, DefUnit, EltTys, FwdDecl);
 
   LexicalBlockStack.pop_back();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3346,6 +3346,10 @@
   CodeGenOpts<"DebugStrictDwarf">, DefaultFalse,
   PosFlag, NegFlag, BothFlags<[CoreOption]>>,
   Group;
+defm incomplete_types : BoolOption<"g", "incomplete-types",
+  CodeGenOpts<"DebugIncompleteTypes">, DefaultFalse,
+  PosFlag, NegFlag, BothFlags<[CoreOption]>>,
+  Group;
 defm column_info : BoolOption<"g", "column-info",
   CodeGenOpts<"DebugColumnInfo">, DefaultTrue,
   NegFlag, PosFlag, BothFlags<[CoreOption]>>,
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -333,6 +333,8 @@
 VALUE_CODEGENOPT(WarnStackSize , 32, UINT_MAX) ///< Set via 
-fwarn-stack-size.
 CODEGENOPT(NoStackArgProbe, 1, 0) ///< Set when -mno-stack-arg-probe is used
 CODEGENOPT(DebugStrictDwarf, 1, 1) ///< Whether or not to use strict DWARF 
info.
+/// Whether or not to include all members in a type in debug info.
+CODEGENOPT(DebugIncompleteTypes, 1, 0)
 
 /// Control the Assignment Tracking debug info feature.
 ENUM_CODEGENOPT(AssignmentTrackingMode, AssignmentTrackingOpts, 2, 
AssignmentTrackingOpts::Disabled)


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ 

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added a reviewer: probinson.
Herald added a project: All.
dblaikie requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

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:
#include "a.h"
void f1() {

  t1::f1<0>();
  ns::f1<0>();

}
b.cpp:
#include "a.h"
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)

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152017

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGenCXX/debug-info-incomplete-types.cpp
  clang/test/Driver/debug-options.c


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -259,6 +259,9 @@
 // RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
 // RUN: %clang -### -c -fdebug-ranges-base-address 
-fno-debug-ranges-base-address %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
 //
+// RUN: %clang -### -c -gincomplete-types %s 2>&1 | FileCheck 
-check-prefix=INCTYPES %s
+// RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NOINCTYPES %s
+//
 // RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=NOPUB %s
 // RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck 
-check-prefix=NOPUB %s
 //
@@ -398,6 +401,9 @@
 // RNGBSE: -fdebug-ranges-base-address
 // NORNGBSE-NOT: -fdebug-ranges-base-address
 //
+// INCTYPES: -gincomplete-types
+// NOINCTYPES-NOT: -gincomplete-types
+//
 // GARANGE-DAG: -generate-arange-section
 //
 // FDTS: "-mllvm" "-generate-type-units"
Index: clang/test/CodeGenCXX/debug-info-incomplete-types.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/debug-info-incomplete-types.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -debug-info-kind=limited -gincomplete-types %s -emit-llvm 
-o - | FileCheck %s
+
+struct t1 {
+  void f1();
+  void f2();
+};
+
+void t1::f1() { }
+
+// CHECK: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1"
+// CHECK-SAME: elements: [[ELEMENTS:![0-9]+]]
+// CHECK: [[ELEMENTS]] = !{}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4308,6 +4308,12 @@
  DebuggerTuning != llvm::DebuggerKind::DBX)))
 CmdArgs.push_back("-gno-column-info");
 
+  if (const Arg *A = Args.getLastArg(options::OPT_gincomplete_types))
+(void)checkDebugInfoOption(A, Args, D, TC);
+  if 

[PATCH] D151952: [clang] adds `__type_pack_index` so we can get a type's parameter pack index

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:5562
+  if (IsDependent)
+goto Return;
+

erichkeane wrote:
> Oh, please don't do this.
perhaps another way to do this if you want to avoid repeating the return 
expression would be a small lambda that contains the return expression and uses 
a simple name - so the returns can just be `return ret();` ? (or I guess nest 
the switch in an `if`, or in another function that uses ref parameters to 
populate the result)

Though in this case it's only twice, so I'd probably repeat the expression?



Comment at: clang/lib/Sema/SemaExprCXX.cpp:5591
+  default:
+llvm_unreachable("unhandled type trait (usualy deliberate)");
+  }

erichkeane wrote:
> What do you mean by `usually deliberate` here? This is a message users will 
> see, so telling them an assertion is deliberate seems incorrect? 
I second the question - though I'd push back on the "This is a message users 
will see" - unreachables won't be seen by users, they get lowered to UB in 
release builds - the text isn't in the program anymore. The text is only for 
Clang developers (and/or clang-as-library users).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151952/new/

https://reviews.llvm.org/D151952

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

[PATCH] D151952: [clang] adds `__type_pack_index` so we can get a type's parameter pack index

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:796-802
+/// If this expression is not value-dependent, this stores the value.
+unsigned Value : 8;
 
 /// The number of arguments to this type trait. According to [implimits]
 /// 8 bits would be enough, but we require (and test for) at least 16 bits
 /// to mirror FunctionType.
+unsigned NumArgs : 16;

cjdb wrote:
> These numbers feel very low for how this modification is using them. Perhaps 
> they should both be bumped to 32?
clang does, I think, have specific implementation limits for this sort of thing 
- 

Well, maybe we don't actually enforce a limit on number of template 
arguments... https://godbolt.org/z/accncYson compiles successfully, and has 
2^18 parameters, beyond the 2^16 suggested here.

But maybe 2^16 is just what we test for/somewhat guarantee.

But if the `Value` is going to be the index into the args, shouldn't it be the 
same size as `NumArgs`, at least? (& the comment says 16, so 16 for both Value 
and NumArgs would seem suitably symmetric, and no larger than the current 
situation - since it'd just be splitting NumArgs in half, while still meeting 
the comment's suggestion/requirements?)

An assert, at least, when populating NumArgs to ensure it's no larger might not 
hurt... (which might be reachable from code/not checked prior - so it could be 
a hard compilation error, I guess, but I wouldn't feel super strongly about it)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151952/new/

https://reviews.llvm.org/D151952

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

[PATCH] D151162: Add -Wpacked-non-pod to -Wall

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGenCXX/warn-padded-packed.cpp:5
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpacked-non-pod -verify=top %s 
-emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpacked-non-pod 
-fclang-abi-compat=15 %s -emit-llvm-only
+

SlaterLatiao wrote:
> denik wrote:
> > Probably we don't need this one. But I'm not sure if there is any impact of 
> > the abi on`packed-non-pod`. According to the original test it checks only 
> > `-Wpacked` `packed attribute is unnecessary` warning.
> I added this line to make sure all the `packed attribute is unnecessary` 
> warnings in the existing test of  `-Wpacked` are not reported by  
> `-Wpacked-non-pod`. Yeah if the abi doesn't have any impact on 
> `-Wpacked-non-pod` this test is unnecessary. 
the old ABI shouldn't emit the `-Wpacked-non-pod` warning since the warning 
isn't correct in the old ABI - where packed is applied to non-pod equally as 
pod.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151162/new/

https://reviews.llvm.org/D151162

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

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

yeah, doesn't look like it found any bugs there (the `arrow` one seems the most 
non-trivwial, but I'm guessing the code as-is is correct), so seems a bit 
questionable as an addition

But @aaron.ballman if you figure this falls more under the "minor tweak to 
existing diagnostic" I won't stand in the way of it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

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

[PATCH] D150411: [NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

(& I realize I'm a bit late to the party, but:

> In several cases it's not completely obvious to me whether a copy assignment 
> operator should or can be defined. But perhaps this doesn't need to be 
> addressed right now: we seem to compile with -Wextra, which contains 
> -Wdeprecated-copy. That should warn if a compiler-generated copy operation is 
> used while another is user-declared.



> The rules for when the copy vs assignment operators are implicitly defined as 
> deleted can be hard to remember. I think being explicit is useful if only to 
> indicate that the author thought about whether such operations should be 
> provided. Since we don't have a principled reason for defining these 
> operations as deleted, it might not be a bad idea to add a comment that 
> states something like "The copy/move assignment operator is defined as 
> deleted pending further motivation".

I'd also vote in favor of the "let it be implicitly deleted - we have the 
warnings enabled to catch it". But don't feel strongly enough to argue that 
chunks of this patch should be reverted)




Comment at: clang/lib/Sema/SemaAccess.cpp:207
+
+// The copy constrcutor and copy assignment operator is defined as deleted
+// pending further motivation.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150411/new/

https://reviews.llvm.org/D150411

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

[PATCH] D143984: [DebugMetadata] Simplify handling subprogram's retainedNodes field. NFCI (1/7)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D143984#4358115 , @dzhidzhoev 
wrote:

> In D143984#4357517 , @dblaikie 
> wrote:
>
>> Looks like this series got approved - is it waiting on any particular 
>> feedback, or do you need someone to commit it for you?
>
> I had been waiting for approval for the change of  
> https://reviews.llvm.org/D144008. How would you suggest landing these commits 
> - individually or all at once?

Ah, fair enough.

It's a tough balance - committing them all at once can make it difficult to see 
which specific change is the cause of a regression/buildbot failure/etc. But 
committing them too far apart can cause large chunks of breakage/harder to 
revert, etc. So... probably close-ish together, let the bots give you some 
results before pushing the next patch, maybe?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143984/new/

https://reviews.llvm.org/D143984

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

[PATCH] D151162: Add -Wpacked-non-pod to -Wall

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Yeah, looks consistent with GCC ( https://godbolt.org/z/EGc3Ec9YT ), as you say 
- so I'm OK with it. But wouldn't mind a second opinion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151162/new/

https://reviews.llvm.org/D151162

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

[PATCH] D148851: Disable llvm-symbolizer on some of the driver tests that are timing out

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D148851#4357360 , @arsenm wrote:

> I also think we need to revert or disable 
> https://github.com/llvm/llvm-project/commit/cead4eceb01b935fae07bf4a7e91911b344d2fec
>
> The symbolizer is unusably slow with it

I believe that issue's been addressed by 
02e8eb1a438bdb1dc9a97aea75a8c9c748048039 
 / D145009 
 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148851/new/

https://reviews.llvm.org/D148851

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

[PATCH] D148851: Disable llvm-symbolizer on some of the driver tests that are timing out

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D148851#4285705 , @dblaikie wrote:

>> I think we probably should add LLVM_DISABLE_SYMBOLIZATION=1 to the lit 
>> level, not in individual tests.
>
> Though I'm not sure how  to do that in a way that it doesn't apply to test 
> that are genuinely failing, where a symbolized backtrace is 
> helpful/important/exactly what want?

Ping @MaskRay thoughts on the tension between disabling it entirely, and it 
being useful for developers investigating real failures?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148851/new/

https://reviews.llvm.org/D148851

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

[PATCH] D143984: [DebugMetadata] Simplify handling subprogram's retainedNodes field. NFCI (1/7)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Looks like this series got approved - is it waiting on any particular feedback, 
or do you need someone to commit it for you?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143984/new/

https://reviews.llvm.org/D143984

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

[PATCH] D149677: [clang][TypePrinter] Add option to skip over elaborated types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D149677#4356863 , @aaron.ballman 
wrote:

> In D149677#4350337 , @li.zhe.hua 
> wrote:
>
>> In D149677#4349523 , 
>> @aaron.ballman wrote:
>>
>>> In D149677#4320178 , @li.zhe.hua 
>>> wrote:
>>>
 Some additional context: I'm working on a refactoring tool to, in part, 
 deprecate an alias by replacing it with its definition. That functionality 
 by itself could be provided through something like a clang-tidy, so I say 
 "initially".
>>>
>>> We typically don't add new printing policies in tree without some need for 
>>> them in the project itself (otherwise this becomes a maintenance problem), 
>>> and I'm not certain I see a strong need for this to live in Clang. I think 
>>> it might be best to wait until you're closer to having a clang-tidy check 
>>> that would use this functionality before we continue with this patch. WDYT?
>>
>> I believe there is value for having this option, as without it, the 
>> `PrintingCallbacks::isScopeVisible` callback is effectively useless in 
>> practice. It won't be triggered in non-canonical types because the 
>> `ElaboratedType` represents a fixed qualification on the name.
>>
>> Note: there may be an argument for folding this functionality directly into 
>> `FullyQualifiedName` (which currently affects template-ids despite the 
>> comment on it suggesting it only is for function names). If the template 
>> name was elaborated, `FullyQualifiedName` does nothing, which seems 
>> inconsistent/incorrect.
>
> Adding @dblaikie as he might have some opinions here.

Not sure I'm quite understanding everything going on here, but I think it's a 
reasonable request that the change be observable in some way in addition 
to/other than a unit test. If not, yeah, maybe wait until it can be/the work 
where it is used is at least up on phab.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149677/new/

https://reviews.llvm.org/D149677

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

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D147844#4329497 , @aaron.ballman 
wrote:

> In general, I think this is incremental progress on the diagnostic behavior. 
> However, it's clear that there is room for interpretation on what is or is 
> not a false positive diagnostic for this,

I hope we can agree on what a false positive is here - when the warning fires 
but the code is what the developer intended (ie: the existing code with the 
existing language semantics produce the desired result, the "fix" is to add 
parentheses that explicitly encode the language's existing rules/behavior 
anyway).

Not that we don't have warnings that do this - that encourage parens to 
reinforce what the language already does to be more explicit/intentional about 
it, and in some cases it's not that uncommon that the user will be adding 
parens that reinforce the precedence rules anyway.

Like, I think all the fixes in libc++, llvm, etc, are false positives? (none of 
them found bugs/unintended behavior) Are there any examples of bugs being found 
by this warning in a codebase? (& how many false positives in such a codebase 
did it also flag?)

> so we should pay close attention to user feedback during the 17.x release 
> cycle. If it seems we're getting significant push back, we may need to come 
> back and rethink.
>
> Specifically, I think we've improved the situation for code like this:
>
>   // `p` is a pointer, `x` is an `int`, and `b` is a `bool`
>   p + x ? 1 : 2; // previously didn't warn, now warns
>   p + b ? 1 : 2; // always warned
>
> Does anyone feel we should not move forward with accepting the patch in its 
> current form?

*goes to look*

Mixed feelings - you're right, incremental improvement/adding missed cases to 
an existing warning (especially if that warning's already stylistic in nature) 
is a lower bar/doesn't necessarily need the false positive assessment. But it 
looks like this case might've been intentionally suppressed in the initial 
warning implementation? (anyone linked to the original warning 
implementation/review/design to check if this was intentional?)

But, yeah, seems marginal enough I don't have strong feelings either way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147844/new/

https://reviews.llvm.org/D147844

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D149193#4328553 , @MaskRay wrote:

> In D149193#4328452 , @dblaikie 
> wrote:
>
>>> I agree that for most(all?) split DWARF users will not see any difference 
>>> since they always use -c -o and don't combine compilation and linking in 
>>> one command.
>>
>> Given that, I'm not sure that this is worth implementing, but if it suits 
>> you I guess.
>
> The split DWARF users are about production users.
>
> There are command line users who do `clang -g -gsplit-dwarf a.c b.c -o x` and 
> I have heard about questions that the `.dwo` files go to strange directories 
> (usually `/tmp`) quite a few times.
> Fixing `-gsplit-dwarf` helps set a baseline for other options like 
> `-ftime-trace` (https://github.com/llvm/llvm-project/issues/57285).

Fair enough


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149193/new/

https://reviews.llvm.org/D149193

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

[PATCH] D149193: [Driver] Add -dumpdir and change -gsplit-dwarf .dwo names for linking

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

> I agree that for most(all?) split DWARF users will not see any difference 
> since they always use -c -o and don't combine compilation and linking in one 
> command.

Given that, I'm not sure that this is worth implementing, but if it suits you I 
guess.




Comment at: clang/lib/Driver/Driver.cpp:3884
+  nullptr, getOpts().getOption(options::OPT_dumpdir),
+  Args.MakeArgString(Args.getLastArgValue(options::OPT_o, "a") + "-"));
+  Arg->claim();

MaskRay wrote:
> scott.linder wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > would be nice to have this "a" derive from wherever we hardcode "a.out" 
> > > > as the default output rather than independently hardcoded here?
> > > > 
> > > > & what does GCC do when the `-o` value has a `.` in it? (if you use `-o 
> > > > a.out` do you get the same `a-x.dwo` behavior, or do you get 
> > > > `a.out-x.dwo`?)
> > > We can use `llvm::sys::path::stem(getDefaultImageName())`, but I feel 
> > > that this just complicates the code.
> > > The default is `a.out` or `a.exe`. If a downstream platform decides to 
> > > deviate and use another filename, say, `b.out`. We will use `-dumpdir b-` 
> > > on this platform and `-dumpdir a-` on everything else. I think they will 
> > > likely be fine with `a-` even if they don't use `a` as the stem name of 
> > > the default image...
> > > 
> > > GCC generally doesn't special case `.` in `-o` for linking, but the 
> > > `a.out` filename is different.
> > > 
> > > ```
> > > gcc -g -gsplit-dwarf d/a.c.c -o e/x.out  # e/x.out-a.dwo
> > > gcc -g -gsplit-dwarf d/a.c.c -o e/a.out  # e/a-a.dwo
> > > ```
> > > 
> > > I think Clang should not special case `a.out`.
> > GCC distinguishes between "dump" and "auxiliary" outputs, and in this case 
> > I think the "dump" outputs retain the basename-suffix (i.e. you get 
> > a.out) whereas "auxiliary" outputs first strip the basename-suffix 
> > (i.e. you get a). The basename-suffix itself can be specified 
> > explicitly via -dumpbase-ext, but it is inferred by default.
> > 
> > The naming for things adds to the for me:
> > 
> > * `-dumpdir` doesn't specifically/exclusively specify a "directory", it 
> > just specifies a prefix
> > * `-dumpbase-ext` only affects the output of non-dump, auxiliary files
> > 
> > I do worry that being close-but-not-quite like GCC here will cause 
> > headaches for someone, but I am also not excited about implementing the 
> > complexity of the GCC options.
> > ... I think the "dump" outputs retain the basename-suffix (i.e. you get 
> > a.out) whereas "auxiliary" outputs first strip the basename-suffix 
> > (i.e. you get a).
> 
> Confirmed.
> ```
> gcc -g -fdump-rtl-all -gsplit-dwarf d/a.c -o e/x.out
> ls e/x.out-a.c.338r.dfinish e/x.out-a.dwo
> ```
> 
> For dump output files, I think they all reside in developer options 
> (https://gcc.gnu.org/onlinedocs/gcc/Developer-Options.html) not intended to 
> be used by end users. They occasionally make incompatible changes in this 
> area as well, e.g. 
> https://gcc.gnu.org/pipermail/gcc-patches/2020-May/546009.html simplified 
> some rules.
> 
> It seems that if we just implement `-dumpdir`, we have gotten the good parts 
> and we are probably done :)
> We can use llvm::sys::path::stem(getDefaultImageName()), but I feel that this 
> just complicates the code.

I think it'd be a bit better this way - otherways "a" looks pretty arbitrary. 
Is there some other reason it would be "a"? If it is derived from "a.out" I 
think having the code reflect that is helpful to the reader.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149193/new/

https://reviews.llvm.org/D149193

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

[PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Got a link to a design discussion motivating this change?

I'd have thought it made sense to put modulemaps in subdirectories - since they 
cover the whole directory, putting them in the root of an include path would be 
problematic if there are multiple distinct projects in that same path. And I 
didn't think this involved searching through subdirectories, but walking up 
parent directories from the included file...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148776/new/

https://reviews.llvm.org/D148776

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

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:109-113
+Warnings
+
+- Address a false positive in ``-Wpacked`` when applied to a non-pod type using
+  Clang ABI >= 15 (fixes 
`#62353`_,
+  fallout from the non-POD packing ABI fix in LLVM 15)

dblaikie wrote:
> aaron.ballman wrote:
> > I think this should move down to `Improvements to Clang's diagnostics` 
> > instead of adding a new category for it.
> Ah, hadn't spotted that category - thanks!
Failed to include the fix there in the initial commit, but got it in 793c5b1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149182/new/

https://reviews.llvm.org/D149182

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

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:109-113
+Warnings
+
+- Address a false positive in ``-Wpacked`` when applied to a non-pod type using
+  Clang ABI >= 15 (fixes 
`#62353`_,
+  fallout from the non-POD packing ABI fix in LLVM 15)

aaron.ballman wrote:
> I think this should move down to `Improvements to Clang's diagnostics` 
> instead of adding a new category for it.
Ah, hadn't spotted that category - thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149182/new/

https://reviews.llvm.org/D149182

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

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

[David Blaikie via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
dblaikie marked an inline comment as done.
Closed by commit rGa8b0c6fa28ac: Remove -Wpacked false positive for non-pod 
types where the layout isnt… (authored by dblaikie).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149182/new/

https://reviews.llvm.org/D149182

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp

Index: clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
===
--- clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
+++ clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
@@ -6,6 +6,8 @@
 // RUN: -fdump-record-layouts -fsyntax-only -verify -x c++ < %s | \
 // RUN:   FileCheck %s
 
+// expected-no-diagnostics
+
 struct A {
   double d;
 };
@@ -14,7 +16,7 @@
   char x[8];
 };
 
-struct [[gnu::packed]] C : B, A { // expected-warning{{packed attribute is unnecessary for 'C'}}
+struct [[gnu::packed]] C : B, A {
   char x alignas(4)[8];
 };
 
Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,top %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,abi15 -fclang-abi-compat=15 %s -emit-llvm-only
 
 struct S1 {
   char c;
@@ -154,7 +155,7 @@
   char c1;
   short s1;
   char c2;
-  S28_non_pod p1; // expected-warning {{not packing field 'p1' as it is non-POD for the purposes of layout}}
+  S28_non_pod p1; // top-warning {{not packing field 'p1' as it is non-POD for the purposes of layout}}
 } __attribute__((packed));
 
 struct S29_non_pod_align_1 {
@@ -167,6 +168,16 @@
 } __attribute__((packed)); // no warning
 static_assert(alignof(S29) == 1, "");
 
+struct S30 {
+protected:
+ short s;
+} __attribute__((packed)); // no warning
+struct S30_use { // abi15-warning {{packed attribute is unnecessary for 'S30_use'}}
+  char c;
+  S30 u;
+} __attribute__((packed));
+static_assert(sizeof(S30_use) == 3, "");
+
 // The warnings are emitted when the layout of the structs is computed, so we have to use them.
 void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*,
S14*, S15*, S16*, S17*, S18*, S19*, S20*, S21*, S22*, S23*, S24*, S25*,
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2198,11 +2198,19 @@
   << (InBits ? 1 : 0); // (byte|bit)
 }
 
+const auto *CXXRD = dyn_cast(RD);
+
 // Warn if we packed it unnecessarily, when the unpacked alignment is not
 // greater than the one after packing, the size in bits doesn't change and
 // the offset of each field is identical.
+// Unless the type is non-POD (for Clang ABI > 15), where the packed
+// attribute on such a type does allow the type to be packed into other
+// structures that use the packed attribute.
 if (Packed && UnpackedAlignment <= Alignment &&
-UnpackedSizeInBits == getSizeInBits() && !HasPackedField)
+UnpackedSizeInBits == getSizeInBits() && !HasPackedField &&
+(!CXXRD || CXXRD->isPOD() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15))
   Diag(D->getLocation(), diag::warn_unnecessary_packed)
   << Context.getTypeDeclType(RD);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -111,6 +111,12 @@
 - Implemented `DR2397 `_ which allows ``auto`` specifier for pointers
   and reference to arrays.
 
+Warnings
+
+- Address a false positive in ``-Wpacked`` when applied to a non-pod type using
+  Clang ABI >= 15 (fixes `#62353`_,
+  fallout from the non-POD packing ABI fix in LLVM 15)
+
 C Language Changes
 --
 - Support for outputs from asm goto statements along indirect edges has been
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141451: [clang] report inlining decisions with -Wattribute-{warning|error}

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D141451#4312256 , @aaron.ballman 
wrote:

> In D141451#4311199 , @dblaikie 
> wrote:
>
>>> probably too much, but then I wonder "how do we make Fortify work?".
>>
>> (I'm still sort of on the side of "if the compile-time costs to get more 
>> verbose diagnostics is high, we could have a low-quality default-on 
>> diagnostic and it could mention/recommend a different/high 
>> quality/compile-time costly diagnostic" so for code bases like the Linux 
>> kernel that rely on this sort of thing a lot, with lots of always_inlining, 
>> etc, they can tradeoff toward the better diagnostic experience and codebases 
>> that don't use these features much they can still get the checking for when 
>> it does come up, but at some cost of quality/awkwardness in needing to 
>> rebuild with other flags)
>
> This isn't about verbosity of the diagnostics, though, so much as it's about 
> user experience. I'd agree that enabling more verbose diagnostic checking is 
> reasonable to ask users to opt into, but what I'm struggling with is 
> expecting users to opt into a mode so that the diagnostics appear in the 
> correct places instead of detached from the source they're noting. Also, with 
> the `error` and `warning` attributes both being used by glibc (and not just 
> for fortify source, but as a general part of the interface), I think this is 
> broader than just inlining decision notes.
>
>> But if folks decided some more invasive tracking was worth implementing 
>> (alongside the debug info codepath that already exists but is perhaps too 
>> slow to be always on) I wouldn't get up on a soapbox to strenuously object - 
>> I just think it's a bit unfortunate to build a duplicate thing, but maybe 
>> necessary.
>
> Agreed on the unfortunateness of duplicating functionality; if we can avoid 
> that, it'd be best. But I'm not sure we have more ideas on how to accomplish 
> it, either.

Fair enough. Well, hopefully somewhere in the summary of all this is a perf 
comparison - between the most narrowed down option using the existing DebugLocs 
and this new/distinct tracking system. But guess it's going that way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141451/new/

https://reviews.llvm.org/D141451

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

[PATCH] D148851: Disable llvm-symbolizer on some of the driver tests that are timing out

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D148851#4311266 , @ahatanak wrote:

> Disable llvm-symbolizer when running lit tests.

This seems problematic though - when lit tests fail it's quite helpful to get a 
symbolized stack trace.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148851/new/

https://reviews.llvm.org/D148851

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

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

[David Blaikie via Phabricator via cfe-commits]

dblaikie marked an inline comment as done.
dblaikie added a comment.

In D149182#4312202 , @aaron.ballman 
wrote:

> Precommit CI found a valid issue (at least on Debian, the Windows failure 
> appears to be unrelated but it's really hard to tell from that output).

Hmm, right ,yeah, valid test failure - fixed.

> Also, this should have a release note, right?

Happy to add one. (done)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149182/new/

https://reviews.llvm.org/D149182

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

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 519247.
dblaikie added a comment.

Describe condition in comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149182/new/

https://reviews.llvm.org/D149182

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp

Index: clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
===
--- clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
+++ clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
@@ -6,6 +6,8 @@
 // RUN: -fdump-record-layouts -fsyntax-only -verify -x c++ < %s | \
 // RUN:   FileCheck %s
 
+// expected-no-diagnostics
+
 struct A {
   double d;
 };
@@ -14,7 +16,7 @@
   char x[8];
 };
 
-struct [[gnu::packed]] C : B, A { // expected-warning{{packed attribute is unnecessary for 'C'}}
+struct [[gnu::packed]] C : B, A {
   char x alignas(4)[8];
 };
 
Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,top %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,abi15 -fclang-abi-compat=15 %s -emit-llvm-only
 
 struct S1 {
   char c;
@@ -154,7 +155,7 @@
   char c1;
   short s1;
   char c2;
-  S28_non_pod p1; // expected-warning {{not packing field 'p1' as it is non-POD for the purposes of layout}}
+  S28_non_pod p1; // top-warning {{not packing field 'p1' as it is non-POD for the purposes of layout}}
 } __attribute__((packed));
 
 struct S29_non_pod_align_1 {
@@ -167,6 +168,16 @@
 } __attribute__((packed)); // no warning
 static_assert(alignof(S29) == 1, "");
 
+struct S30 {
+protected:
+ short s;
+} __attribute__((packed)); // no warning
+struct S30_use { // abi15-warning {{packed attribute is unnecessary for 'S30_use'}}
+  char c;
+  S30 u;
+} __attribute__((packed));
+static_assert(sizeof(S30_use) == 3, "");
+
 // The warnings are emitted when the layout of the structs is computed, so we have to use them.
 void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*,
S14*, S15*, S16*, S17*, S18*, S19*, S20*, S21*, S22*, S23*, S24*, S25*,
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2198,11 +2198,19 @@
   << (InBits ? 1 : 0); // (byte|bit)
 }
 
+const auto *CXXRD = dyn_cast(RD);
+
 // Warn if we packed it unnecessarily, when the unpacked alignment is not
 // greater than the one after packing, the size in bits doesn't change and
 // the offset of each field is identical.
+// Unless the type is non-POD (for Clang ABI > 15), where the packed
+// attribute on such a type does allow the type to be packed into other
+// structures that use the packed attribute.
 if (Packed && UnpackedAlignment <= Alignment &&
-UnpackedSizeInBits == getSizeInBits() && !HasPackedField)
+UnpackedSizeInBits == getSizeInBits() && !HasPackedField &&
+(!CXXRD || CXXRD->isPOD() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15))
   Diag(D->getLocation(), diag::warn_unnecessary_packed)
   << Context.getTypeDeclType(RD);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -106,6 +106,12 @@
 - Implemented `DR2397 `_ which allows ``auto`` specifier for pointers
   and reference to arrays.
 
+Warnings
+
+- Address a false positive in ``-Wpacked`` when applied to a non-pod type using
+  Clang ABI >= 15 (fixes `#62353`_,
+  fallout from the non-POD packing ABI fix in LLVM 15)
+
 C Language Changes
 --
 - Support for outputs from asm goto statements along indirect edges has been
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149182: Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 519245.
dblaikie added a comment.

Release note
Restrict change to Clang ABI 16 and above (& test that condition)
Fix AIX test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149182/new/

https://reviews.llvm.org/D149182

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp
  clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp


Index: clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
===
--- clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
+++ clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
@@ -6,6 +6,8 @@
 // RUN: -fdump-record-layouts -fsyntax-only -verify -x c++ < %s | \
 // RUN:   FileCheck %s
 
+// expected-no-diagnostics
+
 struct A {
   double d;
 };
@@ -14,7 +16,7 @@
   char x[8];
 };
 
-struct [[gnu::packed]] C : B, A { // expected-warning{{packed attribute is 
unnecessary for 'C'}}
+struct [[gnu::packed]] C : B, A {
   char x alignas(4)[8];
 };
 
Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify %s 
-emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked 
-verify=expected,top %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked 
-verify=expected,abi15 -fclang-abi-compat=15 %s -emit-llvm-only
 
 struct S1 {
   char c;
@@ -154,7 +155,7 @@
   char c1;
   short s1;
   char c2;
-  S28_non_pod p1; // expected-warning {{not packing field 'p1' as it is 
non-POD for the purposes of layout}}
+  S28_non_pod p1; // top-warning {{not packing field 'p1' as it is non-POD for 
the purposes of layout}}
 } __attribute__((packed));
 
 struct S29_non_pod_align_1 {
@@ -167,6 +168,16 @@
 } __attribute__((packed)); // no warning
 static_assert(alignof(S29) == 1, "");
 
+struct S30 {
+protected:
+ short s;
+} __attribute__((packed)); // no warning
+struct S30_use { // abi15-warning {{packed attribute is unnecessary for 
'S30_use'}}
+  char c;
+  S30 u;
+} __attribute__((packed));
+static_assert(sizeof(S30_use) == 3, "");
+
 // The warnings are emitted when the layout of the structs is computed, so we 
have to use them.
 void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*,
S14*, S15*, S16*, S17*, S18*, S19*, S20*, S21*, S22*, S23*, S24*, S25*,
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2198,11 +2198,16 @@
   << (InBits ? 1 : 0); // (byte|bit)
 }
 
+const auto *CXXRD = dyn_cast(RD);
+
 // Warn if we packed it unnecessarily, when the unpacked alignment is not
 // greater than the one after packing, the size in bits doesn't change and
 // the offset of each field is identical.
 if (Packed && UnpackedAlignment <= Alignment &&
-UnpackedSizeInBits == getSizeInBits() && !HasPackedField)
+UnpackedSizeInBits == getSizeInBits() && !HasPackedField &&
+(!CXXRD || CXXRD->isPOD() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15))
   Diag(D->getLocation(), diag::warn_unnecessary_packed)
   << Context.getTypeDeclType(RD);
   }
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -106,6 +106,12 @@
 - Implemented `DR2397 `_ which allows ``auto`` 
specifier for pointers
   and reference to arrays.
 
+Warnings
+
+- Address a false positive in ``-Wpacked`` when applied to a non-pod type using
+  Clang ABI >= 15 (fixes 
`#62353`_,
+  fallout from the non-POD packing ABI fix in LLVM 15)
+
 C Language Changes
 --
 - Support for outputs from asm goto statements along indirect edges has been


Index: clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
===
--- clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
+++ clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp
@@ -6,6 +6,8 @@
 // RUN: -fdump-record-layouts -fsyntax-only -verify -x c++ < %s | \
 // RUN:   FileCheck %s
 
+// expected-no-diagnostics
+
 struct A {
   double d;
 };
@@ -14,7 +16,7 @@
   char x[8];
 };
 
-struct [[gnu::packed]] C : B, A { // expected-warning{{packed attribute is unnecessary for 'C'}}
+struct [[gnu::packed]] C : B, A {
   char x alignas(4)[8];
 };
 
Index: clang/test/CodeGenCXX/warn-padded-packed.cpp