[Lldb-commits] [PATCH] D146041: Fix weirdly apologetic diagnostic messages

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith added a comment.

I have no strong opinions on the merits of this patch in either direction; I 
think the "sorry"s in the Sema diagnostics for regrettable non-conformance make 
Clang marginally friendlier, but they do nothing to actually help people who 
encounter the diagnostic.

FWIW, the relevant guidance on Clang diagnostics is 
https://clang.llvm.org/docs/InternalsManual.html#the-format-string, and that 
would override the LLVM coding guidelines' rules in places where they conflict 
(though in this case there's no conflict).




Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:338
 def err_file_too_large : Error<
-  "sorry, unsupported: file '%0' is too large for Clang to process">;
+  "unsupported: file '%0' is too large for Clang to process">;
 def err_include_too_large : Error<

I think we could drop the "unsupported: " here too. (We're allowed to have 
implementation limits; this isn't a conformance issue.)



Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:341
+  "this include generates a translation unit too large for"
   " Clang to process.">, DefaultFatal;
 def err_unsupported_bom : Error<"%0 byte order mark detected in '%1', but "

Please remove the trailing period while you're fixing the style of this 
diagnostic.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9669
   "by aggregate initialization using default member initializer "
   "is not supported; lifetime of %select{temporary|backing array}0 "
   "will end at the end of the full-expression">, InGroup;

While we're fixing style: in this file we have "is not yet supported", "is not 
supported yet", and "is not supported". We should pick one and use it 
consistently; "is not supported yet" would be my preference.



Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:11-12
 template struct Float {};
-using F1 = Float<1.0f>; // FIXME expected-error {{sorry}}
-using F1 = Float<2.0f / 2>; // FIXME expected-error {{sorry}}
+using F1 = Float<1.0f>; // FIXME expected-error
+using F1 = Float<2.0f / 2>; // FIXME expected-error
 

You should retain a `{{...}}` here with some text from the diagnostic for 
`-verify` to match against. Likewise below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146041

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

[Lldb-commits] [PATCH] D142733: Add _Optional as fast qualifier

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith added a comment.

Including a link to the RFC 
(https://discourse.llvm.org/t/rfc-optional-a-type-qualifier-to-indicate-pointer-nullability/68004/2)
 in each of the patches in this series would be helpful.

Assuming that we want to go in this direction, it seems quite expensive to 
model this as a fast qualifier rather than an extended qualifier. Are these 
annotations expected to be so common that it's better to increase the alignment 
of all types than perform extra allocations and indirections for `_Optional` 
qualifiers? Have you measured the memory impact of increasing the alignment of 
`Type`? I think that should be a prerequisite to adding any new kind of fast 
qualifier, and if we do add such a qualifier, we should select carefully which 
qualifier gets this precious bit in `QualType`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142733

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

[Lldb-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith added a comment.

In D131858#3957630 , @arphaman wrote:

> This change has caused a failure in Clang's stage 2 CI on the green dragon 
> Darwin CI: 
> https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6390/console.
>
>   Assertion failed: (lvaluePath->getType() == elemTy && "Unexpected type 
> reference!"), function readAPValue, file 
> /Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/tools/clang/include/clang/AST/AbstractBasicReader.inc,
>  line 736.

This assert is simply wrong, and I've removed it in 
rG2009f2450532450a99c1a03d5e2c30f478121839 
 -- that 
change should be safe to cherry-pick into the release branch. It's possible for 
the recomputation of the type after deserialization to result in a different 
type than what we saw when serializing, because redeclarations of the same 
entity can use the same type with different sugar -- or even slightly different 
types in some cases, such as when an array bound is added in a redeclaration. 
The dumps of the types provided by @steven_wu confirms that we were just seeing 
a difference in type sugar in this case.

>   Assertion failed: (BlockScope.empty() && CurAbbrevs.empty() && "Block 
> imbalance"), function ~BitstreamWriter, file 
> /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/llvm/include/llvm/Bitstream/BitstreamWriter.h,
>  line 119.

Is this still happening? If so, this looks more serious, and will need further 
investigation.

Can we undo the workaround in https://reviews.llvm.org/D139956 and see if the 
bot is now happy? Or can someone who was seeing problems before (@steven_wu?) 
run a test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[Lldb-commits] [PATCH] D133069: Fix inconsistent target arch when attaching to arm64 binaries on arm64e platforms.

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith added a comment.

Hi, the newly-added test, `TestDynamicLoaderDarwin.py`, fails under asan: 
https://gist.github.com/zygoloid/bf7b21f03d7db966e456b6c365c4635d

Please can you take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133069

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

[Lldb-commits] [PATCH] D117383: [lldb] Expose std::pair children for unordered_map

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp:74
+  if (name.consume_front("std::"))
+name.consume_front("__1::");
+  return name.consume_front(type) && name.startswith("<");

This is not right -- libc++ allows customizing its inline namespace name for 
versioning purpose; it's not `__1` across all deployments. For example, with 
the libc++ unstable ABI, it will likely be something else. To handle this 
properly I think you'll need to look for either `std::name<` or something like 
`std::[a-zA-Z0-9_]*::name<`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117383

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

[Lldb-commits] [PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith accepted this revision.
rsmith added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[Lldb-commits] [PATCH] D111509: [clang] use getCommonSugar in an assortment of places

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith accepted this revision.
rsmith added a comment.

Looks good to me.




Comment at: clang/lib/Sema/SemaExprCXX.cpp:6504-6516
 // If we have function pointer types, unify them anyway to unify their
 // exception specifications, if any.
 if (LTy->isFunctionPointerType() || LTy->isMemberFunctionPointerType()) {
   Qualifiers Qs = LTy.getQualifiers();
   LTy = FindCompositePointerType(QuestionLoc, LHS, RHS,
  /*ConvertArgs*/false);
   LTy = Context.getQualifiedType(LTy, Qs);

Can we remove this `if` now? `getCommonSugaredType` should unify the exception 
specifications so I think this stops being a special case.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:6565-6572
 // If we have function pointer types, unify them anyway to unify their
 // exception specifications, if any.
 if (LTy->isFunctionPointerType() || LTy->isMemberFunctionPointerType()) {
   LTy = FindCompositePointerType(QuestionLoc, LHS, RHS);
   assert(!LTy.isNull() && "failed to find composite pointer type for "
   "canonically equivalent function ptr types");
+  return LTy;

Likewise here, can we remove this `if`?



Comment at: clang/test/SemaObjC/format-strings-objc.m:271
 void testTypeOf(NSInteger dW, NSInteger dH) {
-  NSLog(@"dW %d  dH %d",({ __typeof__(dW) __a = (dW); __a < 0 ? -__a : __a; 
}),({ __typeof__(dH) __a = (dH); __a < 0 ? -__a : __a; })); // expected-warning 
2 {{format specifies type 'int' but the argument has type 'long'}}
+  NSLog(@"dW %d  dH %d",({ __typeof__(dW) __a = (dW); __a < 0 ? -__a : __a; 
}),({ __typeof__(dH) __a = (dH); __a < 0 ? -__a : __a; })); // expected-warning 
2 {{values of type 'NSInteger' should not be used as format arguments; add an 
explicit cast to 'long' instead}}
 }

Not related to this patch, but this new diagnostic is in some ways worse than 
the old one: casting to `long` doesn't fix the problem, given that the format 
specifier here `%d` expects an `int`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111509

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

[Lldb-commits] [PATCH] D95164: Make SBDebugger::CreateTargetWithFileAndArch accept lldb.LLDB_DEFAULT_ARCH

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith added a comment.

I reverted this and the follow-up fix in 
rG8b1171488575bd0b5ccb23bc1a3d22e2aaccb244 
, because 
it's introduced several test failures. Example: 
http://lab.llvm.org:8011/#/builders/96/builds/3777

It appears that these tests use `CreateTargetWithFileAndArch(None, None)`, 
which worked before these changes and not afterwards.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95164

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

[Lldb-commits] [PATCH] D39307: Fix global data symbol resolution

[Richard Smith - zygoloid via Phabricator via lldb-commits]

rsmith added a comment.

In https://reviews.llvm.org/D39307#907358, @xiaobai wrote:

> It seems like clang shouldn't even be asking us about this in the first 
> place, but I don't fully understand why it's trying to do what it's trying to 
> do.


It looks like there are two issues here: clang is doing seemingly-unnecessary 
work as part of a redeclaration lookup, and lldb is reacting badly to clang 
performing those lookups. The former part is possibly a bug, but we sometimes 
really do want to do a full lookup for redeclaration checking, particularly for 
`-Wshadow`. In general we reserve the right to look up names even when it might 
not be obvious why, so a clang API consumer shouldn't be assuming otherwise.

It would seem reasonable to me for lldb to pick the symbol from the current 
module in the case of an ambiguity, and to otherwise make all symbols visible. 
I think that's generally what gdb does. That said... dynamically changing how a 
name resolves might cause some problems, particularly in C++, since Clang 
caches lookup results from `ExternalASTSource`s in the `DeclContext` lookup 
tables. If that turns out to be a problem, I think we'll either need to push 
some part of this disambiguation down into Clang itself or make LLDB invalidate 
those caches when the current module is switched.


https://reviews.llvm.org/D39307



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