RE: [clang] 4821508 - Revert "DebugInfo: Fully integrate ctor type homing into 'limited' debug info"

[Robinson, Paul via cfe-commits]

Hi Dave,

The original commit message included
Also fix a bug I found along the way that was causing ctor type homing
to kick in even when something could be vtable homed

Is it reasonable to fix that without removing ctor homing in general?
Or would that cause too much test churn, as you're planning to recommit
this patch anyway?
--paulr

> -Original Message-
> From: cfe-commits  On Behalf Of David
> Blaikie via cfe-commits
> Sent: Friday, June 24, 2022 1:08 PM
> To: cfe-commits@lists.llvm.org
> Subject: [clang] 4821508 - Revert "DebugInfo: Fully integrate ctor type
> homing into 'limited' debug info"
> 
> 
> Author: David Blaikie
> Date: 2022-06-24T17:07:47Z
> New Revision: 4821508d4db75a535d02b8938f81fac6de66cc26
> 
> URL: https://urldefense.com/v3/__https://github.com/llvm/llvm-
> project/commit/4821508d4db75a535d02b8938f81fac6de66cc26__;!!JmoZiZGBv3RvKR
> Sx!7pmjZG0ponrxAVY0dOSOTgWfvxMgERh3TNpn2zRGr7NTuooxwQKHzTroRX39LtKaKCXGoQD
> n3Ri4BOhJymrwDVc8Rzk$
> DIFF: https://urldefense.com/v3/__https://github.com/llvm/llvm-
> project/commit/4821508d4db75a535d02b8938f81fac6de66cc26.diff__;!!JmoZiZGBv
> 3RvKRSx!7pmjZG0ponrxAVY0dOSOTgWfvxMgERh3TNpn2zRGr7NTuooxwQKHzTroRX39LtKaKC
> XGoQDn3Ri4BOhJymrwKAIx5Rg$
> 
> LOG: Revert "DebugInfo: Fully integrate ctor type homing into 'limited'
> debug info"
> 
> Reverting to simplify some Google-internal rollout issues. Will recommit
> in a week or two.
> 
> This reverts commit 517bbc64dbe493644eff8d55fd9566435e930520.
> 
> Added:
> clang/test/CodeGenCXX/debug-info-ctor-homing-flag.cpp
> 
> Modified:
> clang/docs/UsersManual.rst
> clang/include/clang/Basic/CodeGenOptions.h
> clang/include/clang/Basic/DebugInfoOptions.h
> clang/include/clang/Driver/Options.td
> clang/lib/CodeGen/CGDebugInfo.cpp
> clang/lib/Driver/ToolChains/Clang.cpp
> clang/lib/Frontend/CompilerInvocation.cpp
> clang/test/CodeGen/attr-cpuspecific-renaming.cpp
> clang/test/CodeGen/pr52782-stdcall-func-decl.cpp
> clang/test/CodeGenCXX/debug-info-class.cpp
> clang/test/CodeGenCXX/debug-info-limited-ctor.cpp
> clang/test/CodeGenCXX/debug-info-template-explicit-specialization.cpp
> clang/test/CodeGenCXX/debug-lambda-this.cpp
> clang/test/CodeGenCXX/ibm128-declarations.cpp
> clang/test/CodeGenCXX/standalone-debug-attribute.cpp
> clang/test/Driver/cl-options.c
> clang/test/Driver/clang-g-opts.c
> clang/test/Driver/cuda-dwarf-2.cu
> clang/test/Driver/debug-options-as.c
> clang/test/Driver/debug-options.c
> clang/test/Driver/integrated-as.s
> clang/test/Driver/myriad-toolchain.c
> clang/test/Driver/openmp-offload-gpu.c
> clang/test/Driver/split-debug.c
> clang/test/OpenMP/debug_private.c
> clang/test/OpenMP/debug_task_shared.c
> clang/test/OpenMP/debug_threadprivate_copyin.c
> 
> Removed:
> 
> 
> 
> ##
> ##
> diff  --git a/clang/docs/UsersManual.rst b/clang/docs/UsersManual.rst
> index ccb5fed1cb370..e12dc72407b13 100644
> --- a/clang/docs/UsersManual.rst
> +++ b/clang/docs/UsersManual.rst
> @@ -2672,6 +2672,19 @@ below. If multiple flags are present, the last one
> is used.
> **-fno-standalone-debug** option can be used to get to turn on the
> vtable-based optimization described above.
> 
> +.. option:: -fuse-ctor-homing
> +
> +   This optimization is similar to the optimizations that are enabled as
> part
> +   of -fno-standalone-debug. Here, Clang only emits type info for a
> +   non-trivial, non-aggregate C++ class in the modules that contain a
> +   definition of one of its constructors. This relies on the additional
> +   assumption that all classes that are not trivially constructible have
> a
> +   non-trivial constructor that is used somewhere. The negation,
> +   -fno-use-ctor-homing, ensures that constructor homing is not used.
> +
> +   This flag is not enabled by default, and needs to be used with -cc1 or
> +   -Xclang.
> +
>  .. option:: -g
> 
>Generate complete debug info.
> 
> diff  --git a/clang/include/clang/Basic/CodeGenOptions.h
> b/clang/include/clang/Basic/CodeGenOptions.h
> index 5f5218c87a605..23d76c308d847 100644
> --- a/clang/include/clang/Basic/CodeGenOptions.h
> +++ b/clang/include/clang/Basic/CodeGenOptions.h
> @@ -468,7 +468,7 @@ class CodeGenOptions : public CodeGenOptionsBase {
> 
>/// Check if type and variable info should be emitted.
>bool hasReducedDebugInfo() const {
> -return getDebugInfo() >= codegenoptions::LimitedDebugInfo;
> +return getDebugInfo() >= codegenoptions::DebugInfoConstructor;
>}
> 
>/// Check if maybe unused type info should be emitted.
> 
> diff  --git a/clang/include/clang/Basic/DebugInfoOptions.h
> b/clang/include/clang/Basic/DebugInfoOptions.h
> index 98210cc3cfa13..a99a2b5903d7e 100644
> --- a/clang/include/clang/Basic/DebugInfoOptions.h
> +++ b/clang/include/clang/Basic/DebugInfoOptions.h
> @@ -34,6 +34,12 @@ enum 

RE: Call for an assistance pushing patch review forward

[Robinson, Paul via cfe-commits]

Hi Mateusz,

I wouldn’t worry too much about the failed build.  I took a peek and it looks 
like the failures are mostly in places very unrelated to your patch.  If your 
own testing shows no problems, it’s very likely you’re fine.

Regarding lack of response, this is unfortunately more common than it should 
be.  Our recommended practice—and I’m surprised we don’t say anything on the 
website—is to add a “ping” comment to your review, maybe once a week.  This can 
“bump” it up in someone’s to-be-reviewed list; if nothing else, there’s another 
email to the list that will hopefully catch someone’s attention.

Good luck,
--paulr

From: cfe-commits  On Behalf Of stryku_t 
via cfe-commits
Sent: Thursday, May 26, 2022 4:20 PM
To: cfe-commits@lists.llvm.org
Subject: Call for an assistance pushing patch review forward

Hi,

Some time ago I submitted a Clang patch for review: 
https://reviews.llvm.org/D123532

It's been some time and I can't make progress with it.
I'm aware that there is failed build. But, as I mentioned in one of the 
comments, I can't reproduce it locally.
Tried to reach out to the people mentioned in the review, but I struggle 
getting answers.
At this point I'm not sure if I'm doing something wrong or people are just 
busy. Should tag different people? Different project? Am I expected to figure 
out failed builds on my own before someone will review the changes?

Could someone from the community please assist me how for move forward with 
this patch? Also, I'd be grateful if there's someone who can hint me how to 
reproduce failed builds locally or using some CI server.

Thanks in advance!

Best regards,
Mateusz Janek
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [clang] af1d3e6 - Allow init_priority values <= 100 and > 65535 within system headers.

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits  On Behalf Of Aaron
> Ballman via cfe-commits
> Sent: Wednesday, September 23, 2020 3:27 PM
> To: cfe-commits@lists.llvm.org
> Subject: [clang] af1d3e6 - Allow init_priority values <= 100 and > 65535
> within system headers.
> 
> 
> Author: Aaron Ballman
> Date: 2020-09-23T15:26:50-04:00
> New Revision: af1d3e655991e5f0c86df372b8583a60d20268e0
> 
> URL: https://urldefense.com/v3/__https://github.com/llvm/llvm-
> project/commit/af1d3e655991e5f0c86df372b8583a60d20268e0__;!!JmoZiZGBv3RvKR
> Sx!uCGMaNBytzj1FhG349bRMYFleWUAIQwiwZLW4mUe1SfbBjKnJvDM4dGzlplaS-cftQ$
> DIFF: https://urldefense.com/v3/__https://github.com/llvm/llvm-
> project/commit/af1d3e655991e5f0c86df372b8583a60d20268e0.diff__;!!JmoZiZGBv
> 3RvKRSx!uCGMaNBytzj1FhG349bRMYFleWUAIQwiwZLW4mUe1SfbBjKnJvDM4dGzlpn6UUxJFg
> $
> 
> LOG: Allow init_priority values <= 100 and > 65535 within system headers.
> 
> This also adds some bare-bones documentation for the attribute rather
> than leaving it undocumented.
> 
> Added:
> 
> 
> Modified:
> clang/include/clang/Basic/Attr.td
> clang/include/clang/Basic/AttrDocs.td
> clang/lib/Sema/SemaDeclAttr.cpp
> clang/test/SemaCXX/init-priority-attr.cpp
> 
> Removed:
> 
> 
> 
> ##
> ##
> diff  --git a/clang/include/clang/Basic/Attr.td
> b/clang/include/clang/Basic/Attr.td
> index e8ac819c8b556..7222f396089e0 100644
> --- a/clang/include/clang/Basic/Attr.td
> +++ b/clang/include/clang/Basic/Attr.td
> @@ -2196,7 +2196,7 @@ def InitPriority : InheritableAttr {
>let Spellings = [GCC<"init_priority", /*AllowInC*/0>];
>let Args = [UnsignedArgument<"Priority">];
>let Subjects = SubjectList<[Var], ErrorDiag>;
> -  let Documentation = [Undocumented];
> +  let Documentation = [InitPriorityDocs];
>  }
> 
>  def Section : InheritableAttr {
> 
> diff  --git a/clang/include/clang/Basic/AttrDocs.td
> b/clang/include/clang/Basic/AttrDocs.td
> index 9c119218656d8..3e27924589e4a 100644
> --- a/clang/include/clang/Basic/AttrDocs.td
> +++ b/clang/include/clang/Basic/AttrDocs.td
> @@ -57,6 +57,32 @@ global variable or function should be in after
> translation.
>let Heading = "section, __declspec(allocate)";
>  }
> 
> +def InitPriorityDocs : Documentation {
> +  let Category = DocCatVariable;
> +  let Content = [{
> +In C++, the order in which global variables are initialized across
> translation
> +units is unspecified, unlike the ordering within a single translation
> unit. The
> +``init_priority`` attribute allows you to specify a relative ordering for
> the
> +initialization of objects declared at namespace scope in C++. The
> priority is
> +given as an integer constant expression between 101 and 65535
> (inclusive).
> +Priorities outside of that range are reserved for use by the
> implementation. A
> +lower value indicates a higher priority of initialization. Note that only
> the
> +relative ordering of values is important. For example:
> +
> +.. code-block:: c++
> +
> +  struct SomeType { SomeType(); };
> +  __attribute__((init_priority(200))) SomeType Obj1;
> +  __attribute__((init_priority(101))) SomeType Obj2;
> +
> +``Obj1`` will be initialized *before* ``Obj2`` despite the usual order of
> +initialization being the opposite.

I think here ``Obj2`` will be initialized before ``Obj1``
despite the usual order?  (Usual order would be in order of
declaration, and init_priority goes from low to high.)
--paulr

> +
> +This attribute is only supported for C++ and Objective-C++ and is ignored
> in
> +other language modes.
> +  }];
> +}
> +
>  def InitSegDocs : Documentation {
>let Category = DocCatVariable;
>let Content = [{
> 
> diff  --git a/clang/lib/Sema/SemaDeclAttr.cpp
> b/clang/lib/Sema/SemaDeclAttr.cpp
> index 6edeb79396296..3babbe05ca8f9 100644
> --- a/clang/lib/Sema/SemaDeclAttr.cpp
> +++ b/clang/lib/Sema/SemaDeclAttr.cpp
> @@ -3313,7 +3313,11 @@ static void handleInitPriorityAttr(Sema , Decl
> *D, const ParsedAttr ) {
>  return;
>}
> 
> -  if (prioritynum < 101 || prioritynum > 65535) {
> +  // Only perform the priority check if the attribute is outside of a
> system
> +  // header. Values <= 100 are reserved for the implementation, and
> libc++
> +  // benefits from being able to specify values in that range.
> +  if ((prioritynum < 101 || prioritynum > 65535) &&
> +  !S.getSourceManager().isInSystemHeader(AL.getLoc())) {
>  S.Diag(AL.getLoc(), diag::err_attribute_argument_out_of_range)
>  << E->getSourceRange() << AL << 101 << 65535;
>  AL.setInvalid();
> 
> diff  --git a/clang/test/SemaCXX/init-priority-attr.cpp
> b/clang/test/SemaCXX/init-priority-attr.cpp
> index 8f31e2fd62d00..5b5e3b9eb940e 100644
> --- a/clang/test/SemaCXX/init-priority-attr.cpp
> +++ b/clang/test/SemaCXX/init-priority-attr.cpp
> @@ -1,4 +1,9 @@
>  // RUN: %clang_cc1 -fsyntax-only -verify %s
> +// RUN: %clang_cc1 -fsyntax-only -DSYSTEM -verify %s
> +
> +#if 

RE: [clang-tools-extra] 8e325cf - [clangd] Work around PS4 -fno-exceptions, easier than disabling tests?

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits  On Behalf Of Sam
> McCall via cfe-commits
> Sent: Thursday, May 28, 2020 11:15 AM
> To: cfe-commits@lists.llvm.org
> Subject: [clang-tools-extra] 8e325cf - [clangd] Work around PS4 -fno-
> exceptions, easier than disabling tests?
> 
> 
> Author: Sam McCall
> Date: 2020-05-28T17:14:23+02:00
> New Revision: 8e325cfc1456820e2253909e4aa0c3014f1e050c
> 
> URL: https://github.com/llvm/llvm-
> project/commit/8e325cfc1456820e2253909e4aa0c3014f1e050c
> DIFF: https://github.com/llvm/llvm-
> project/commit/8e325cfc1456820e2253909e4aa0c3014f1e050c.diff
> 
> LOG: [clangd] Work around PS4 -fno-exceptions, easier than disabling
> tests?
> 
> Added:
> 
> 
> Modified:
> clang-tools-extra/clangd/unittests/XRefsTests.cpp
> 
> Removed:
> 
> 
> 
> ##
> ##
> diff  --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-
> tools-extra/clangd/unittests/XRefsTests.cpp
> index b73a310e95fb..e260285a179c 100644
> --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
> +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
> @@ -245,7 +245,9 @@ TEST(HighlightsTest, ControlFlow) {
>};
>for (const char *Test : Tests) {
>  Annotations T(Test);
> -auto AST = TestTU::withCode(T.code()).build();
> +auto TU = TestTU::withCode(T.code());
> +TU.ExtraArgs.push_back("-fexceptions"); // FIXME: stop testing on
> PS4.

Yeah, sorry for the inconvenience.  We haven't tried evaluating clangd
for a PS4 environment but in the meantime it feels like more coverage
is a good thing?
Thanks,
--paulr

> +auto AST = TU.build();
>  EXPECT_THAT(findDocumentHighlights(AST, T.point()),
> HighlightsFrom(T))
>  << Test;
>}
> 
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r321312 - [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

[Robinson, Paul via cfe-commits]

r321457.  Happy new year!
--paulr

> -Original Message-
> From: Kim Gräsman [mailto:kim.gras...@gmail.com]
> Sent: Tuesday, December 26, 2017 1:56 AM
> To: Robinson, Paul
> Cc: cfe-commits
> Subject: Re: r321312 - [AST] Incorrectly qualified unscoped enumeration as
> template actual parameter.
> 
> This broke a test case in IWYU, so I read the diff a few times more
> than usual...
> 
> On Thu, Dec 21, 2017 at 10:47 PM, Paul Robinson via cfe-commits
>  wrote:
> >
> >// scope of the enumeration.
> > -  if (ED->isScoped() || ED->getIdentifier())
> > +  // For the case of unscoped enumerator, do not include in the
> qualified
> > +  // name any information about its enum enclosing scope, as is
> visibility
> > +  // is global.
> 
> 
> Typo: as is -> as its
> 
> Happy holidays,
> - Kim

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

RE: [PATCH] D39622: Fix type name generation in DWARF for template instantiations with enum types and template specializations

[Robinson, Paul via cfe-commits]

On the lldb-dev thread I thought this was a reasonable idea (DW_AT_linkage_name 
on types) but given the use-case, probably best to confine it to classes with 
vtables?  If there's a broader use-case it wasn't clear from the other thread; 
there it was reported that LLDB really only uses the mangled name for tracking 
down the type description associated with a vtable (which of course has a 
mangled name giving the type).
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Tuesday, December 19, 2017 1:36 PM
To: Anton Gorenkov; Robinson, Paul; Adrian Prantl
Cc: xgsa; mlek...@skidmore.edu; 
reviews+d39622+public+b0839896b45cd...@reviews.llvm.org; 
cfe-commits@lists.llvm.org; shen...@google.com
Subject: Re: [PATCH] D39622: Fix type name generation in DWARF for template 
instantiations with enum types and template specializations

Not much - I've put them on this part of the thread specifically to raise 
attention.

If it doesn't get visibility here, maybe a cfe-dev thread would be good.


On Tue, Dec 19, 2017 at 1:33 PM Anton Gorenkov 
> wrote:

Sorry, I am quite new to the process. It seems, Adrian and Paul are in the 
reviewers/subscribers list to the original review 
(https://reviews.llvm.org/D39622). Should I do something else?
19.12.2017 23:06, David Blaikie wrote:
Yep, could be worth having a conversation with the GDB folks and/or at least 
poke the other LLVM debug info folks (Adrian and Paul - Paul's pretty 
interesting since he works with/on another (not LLDB nor GDB) debugger which 
would have to think about this functionality/feature/issue/data/limitation)

On Tue, Dec 19, 2017 at 1:04 PM Anton Gorenkov 
> wrote:
There was a discussion in lldb-dev mailing list on this topic and I
suppose a reliable solution was suggested [1]. It is to generate
DW_AT_linkage_name for vtable DIE of a class and provide an additional
accelerator table. I am going to try to implement this approach (it will
require some work on both clang and lldb sides), but I'd like also to
understand if I should discard or complete the current patch. Certainly,
I'd prefer to complete it if it could be applied (I suppose, at least
tests should be added), because even with long term solution implemented
in clang/lldb, gdb still won't resolve dynamic types properly for the
described cases.

[1] - http://lists.llvm.org/pipermail/lldb-dev/2017-December/013048.html

15.12.2017 21:25, David Blaikie via cfe-commits wrote:
>
>
> On Fri, Dec 15, 2017 at 8:09 AM xgsa 
> >> wrote:
>
> David, thank you for the detailed answer and corner cases.
> Just to clarify: everywhere in my mail where I mentioned
> "debugger", I meant LLDB, but not GDB (except, where I mentioned
> GDB explicitly). Currently, I have no plans to work on GDB,
> however I would like to make the clang+LLDB pair working in such
> cases.
>
>
> *nod* My concern is making sure, if possible, we figure out a design
> that seems viable long-term/in general. (& if we figure out what that
> design is, but decide it's not achievable immediately, we can make
> deliberate tradeoffs, document the long term goal & what the short
> term solutions cost relative to that goal, etc)
>
> Thus, I have described your idea in the lldb-dev mailing list [1].
> Still, I have some concerns about the performance of such
> semantically aware matching. Currently, with acceleration tables
> (e.g. apple_types etc) the matching is as fast as lookup in hash
> map and hash map is loade almost without postprocessing.
> Semantically aware matching will require either processing during
> statup or almost linear lookup.
>
>
> Yep, I agree - that seems like a reasonable concern. I wonder whether
> it'd be reasonable to put accelerator table entries containing the
> base name of the template to ease such lookup?
>
>  Still, should this topic be raised in cde-dev or are all the
> interested people already here?
>
>
> Yeah, might be worth moving this to a thread there. Though we probably
> have all the right people here, it's a better spot for the
> conversation even for spectators, history (finding this later when we
> have similar questions, etc), etc.
>
> [1] -
> http://lists.llvm.org/pipermail/lldb-dev/2017-December/013038.html
> 14.12.2017, 22:40, "David Blaikie" 
> 
> >>:
>> On Thu, Dec 14, 2017 at 2:21 AM Anton via Phabricator
>>  
>> >> wrote:
>>
>> xgsa added a comment.
>>
>> In https://reviews.llvm.org/D39622#954585, @probinson wrote:
>>
>> > Philosophically, mangled names and DWARF information serve
>> different 

RE: [clang-tools-extra] r318287 - [clangd] Support returning a limited number of completion results.

[Robinson, Paul via cfe-commits]

Hi Sam,
It looks like llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast has been
failing a couple of clangd tests ever since this went in.

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/13517

Please fix or revert ASAP when bots go red.  Failures like this interfere
with our CI process.

Thanks,
--paulr
PS4 code owner


> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Sam McCall via cfe-commits
> Sent: Wednesday, November 15, 2017 1:16 AM
> To: cfe-commits@lists.llvm.org
> Subject: [clang-tools-extra] r318287 - [clangd] Support returning a
> limited number of completion results.
> 
> Author: sammccall
> Date: Wed Nov 15 01:16:29 2017
> New Revision: 318287
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=318287=rev
> Log:
> [clangd] Support returning a limited number of completion results.
> 
> Summary:
> All results are scored, we only process CodeCompletionStrings for the
> winners.
> We now return CompletionList rather than CompletionItem[] (both are
> valid).
> sortText is now based on CodeCompletionResult::orderedName (mostly the
> same).
> 
> This is the first clangd-only completion option, so plumbing changed.
> It requires a small clangd patch (exposing
> CodeCompletionResult::orderedName).
> 
> (This can't usefully be enabled yet: we don't support server-side
> filtering)
> 
> Reviewers: ilya-biryukov
> 
> Subscribers: cfe-commits
> 
> Differential Revision: https://reviews.llvm.org/D39852
> 
> Modified:
> clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> clang-tools-extra/trunk/clangd/ClangdServer.cpp
> clang-tools-extra/trunk/clangd/ClangdServer.h
> clang-tools-extra/trunk/clangd/ClangdUnit.cpp
> clang-tools-extra/trunk/clangd/ClangdUnit.h
> clang-tools-extra/trunk/clangd/Protocol.cpp
> clang-tools-extra/trunk/clangd/Protocol.h
> clang-tools-extra/trunk/test/clangd/authority-less-uri.test
> clang-tools-extra/trunk/test/clangd/completion-items-kinds.test
> clang-tools-extra/trunk/test/clangd/completion-priorities.test
> clang-tools-extra/trunk/test/clangd/completion-qualifiers.test
> clang-tools-extra/trunk/test/clangd/completion-snippet.test
> clang-tools-extra/trunk/test/clangd/completion.test
> clang-tools-extra/trunk/test/clangd/protocol.test
> clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
> 
> Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-
> extra/trunk/clangd/ClangdLSPServer.cpp?rev=318287=318286=318287
> =diff
> ==
> 
> --- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
> +++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Wed Nov 15 01:16:29
> 2017
> @@ -195,15 +195,15 @@ void ClangdLSPServer::onCodeAction(Ctx C
>  }
> 
>  void ClangdLSPServer::onCompletion(Ctx C, TextDocumentPositionParams
> ) {
> -  auto Items = Server
> -   .codeComplete(Params.textDocument.uri.file,
> - Position{Params.position.line,
> -  Params.position.character})
> -   .get() // FIXME(ibiryukov): This could be made async
> if we
> -  // had an API that would allow to attach
> callbacks to
> -  // futures returned by ClangdServer.
> -   .Value;
> -  C.reply(json::ary(Items));
> +  auto List = Server
> +  .codeComplete(
> +  Params.textDocument.uri.file,
> +  Position{Params.position.line,
> Params.position.character})
> +  .get() // FIXME(ibiryukov): This could be made async if
> we
> + // had an API that would allow to attach
> callbacks to
> + // futures returned by ClangdServer.
> +  .Value;
> +  C.reply(List);
>  }
> 
>  void ClangdLSPServer::onSignatureHelp(Ctx C,
> 
> Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
> URL: http://llvm.org/viewvc/llvm-project/clang-tools-
> extra/trunk/clangd/ClangdServer.cpp?rev=318287=318286=318287=di
> ff
> ==
> 
> --- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
> +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Wed Nov 15 01:16:29
> 2017
> @@ -222,11 +222,11 @@ std::future ClangdServer::forceRep
>   std::move(TaggedFS));
>  }
> 
> -std::future>
> +std::future
>  ClangdServer::codeComplete(PathRef File, Position Pos,
> llvm::Optional OverridenContents,
> IntrusiveRefCntPtr *UsedFS) {
> -  using ResultType = Tagged;
> +  using ResultType = Tagged;
> 
>std::promise ResultPromise;
> 
> @@ -242,11 +242,10 @@ 

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

(though perhaps we had some position that debugger tuning wouldn't ever be the 
only way to access functionality, only change defaults)

That's correct.  Tuning must unpack to other settings that can be set 
separately.  That was very strong feedback from when we introduced the tuning 
concept.
--paulr

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

RE: [PATCH] D36501: add flag to undo ABI change in r310401

[Robinson, Paul via cfe-commits]

Paul: is the PS4 toolchain's ABI based on that of a particular Clang release, 
or is it a branch from trunk at some point? Or something else? (And which 
release / revision?)

I am reminded that there are two parts to the ABI, the C++ part and the 
platform part.
PS4's C++ ABI is whatever Clang 3.2 did; the platform part is AMD64 (as of LLVM 
3.2) with (IIRC) a couple minor differences.

Whenever there is an ABI change (that we notice), we go through an exercise to 
determine whether it's benign or we have to undo it.  Games built with a 
3.2-based toolchain must be runnable on OS/middleware built with all subsequent 
versions.  It is never the case that a game built with version N must be 
runnable on an OS built with version N-1, if that helps.
--paulr
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r296554 - [PS4] Set our default dialect to C++11. NFC for other targets.

[Robinson, Paul via cfe-commits]

Mehdi, again I invite you to discuss this on the cfe-dev thread rather than 
here.  I have posted a reply there to bump it up, and any comments you have 
will be more visible and better serve the community over there.
Thanks,
--paulr

From: mehdi.am...@apple.com [mailto:mehdi.am...@apple.com]
Sent: Wednesday, March 01, 2017 3:52 PM
To: Sean Silva
Cc: Robinson, Paul; cfe-commits
Subject: Re: r296554 - [PS4] Set our default dialect to C++11. NFC for other 
targets.


On Mar 1, 2017, at 3:24 PM, Sean Silva 
> wrote:



On Wed, Mar 1, 2017 at 10:35 AM, Mehdi Amini 
> wrote:
I’m not sure I find this nice to see this upstream.

I not fond in general of this kind of difference in behavior. I don’t think it 
is good for clang to have different default for this kind of settings depending 
on the platform. It does not provide a very good user experience from a 
cross-platform point of view (i.e. my compiler behaves very differently when I 
target one platform instead of another).

What I like about it is that the upstream PS4 bots now test that we don't 
depend on the C++98 default language standard in tests, which is net positive 
IMO since it facilitates future changes. Should this be a point of 
vendor/platform extensibility? That's a question for cfe-dev, but I don't think 
it's unreasonable. (see also: PS4 has -fno-rtti and -fno-exceptions by default, 
even though users already know to pass the right flags and expect to have to).

I rather have the driver rejecting “unsupported configuration”, requiring the 
right flags, instead of diverging the default.

—
Mehdi






On Feb 28, 2017, at 11:22 PM, Sean Silva via cfe-commits 
> wrote:

Nice!

-- Sean Silva

On Tue, Feb 28, 2017 at 5:01 PM, Paul Robinson via cfe-commits 
> wrote:
Author: probinson
Date: Tue Feb 28 19:01:10 2017
New Revision: 296554

URL: http://llvm.org/viewvc/llvm-project?rev=296554=rev
Log:
[PS4] Set our default dialect to C++11. NFC for other targets.
Reapplies r296209 now that r296549 has fixed what really seems to be
the last problematic test.

Modified:
cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=296554=296553=296554=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Tue Feb 28 19:01:10 2017
@@ -1582,7 +1582,11 @@ void CompilerInvocation::setLangDefaults
 case IK_PreprocessedCXX:
 case IK_ObjCXX:
 case IK_PreprocessedObjCXX:
-  LangStd = LangStandard::lang_gnucxx98;
+  // The PS4 uses C++11 as the default C++ standard.
+  if (T.isPS4())
+LangStd = LangStandard::lang_gnucxx11;
+  else
+LangStd = LangStandard::lang_gnucxx98;
   break;
 case IK_RenderScript:
   LangStd = LangStandard::lang_c99;


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

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



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

RE: r296554 - [PS4] Set our default dialect to C++11. NFC for other targets.

[Robinson, Paul via cfe-commits]

Probably better to debate this on the cfe-dev thread "Setting default dialect 
to C++11" than here.
That said, it's far from the only target-dependent setting in the driver.  
There are things ranging from the default DWARF version to whether exceptions 
are on by default to whether Microsoft extensions are on by default.  
Personally I think mixing things up gets us better coverage; this is why we 
like having bots for a variety of platforms, and it's not just for the obvious 
backend reasons.
--paulr

From: mehdi.am...@apple.com [mailto:mehdi.am...@apple.com]
Sent: Wednesday, March 01, 2017 10:36 AM
To: Sean Silva
Cc: Robinson, Paul; cfe-commits
Subject: Re: r296554 - [PS4] Set our default dialect to C++11. NFC for other 
targets.

I’m not sure I find this nice to see this upstream.

I not fond in general of this kind of difference in behavior. I don’t think it 
is good for clang to have different default for this kind of settings depending 
on the platform. It does not provide a very good user experience from a 
cross-platform point of view (i.e. my compiler behaves very differently when I 
target one platform instead of another).

—
Mehdi



On Feb 28, 2017, at 11:22 PM, Sean Silva via cfe-commits 
> wrote:

Nice!

-- Sean Silva

On Tue, Feb 28, 2017 at 5:01 PM, Paul Robinson via cfe-commits 
> wrote:
Author: probinson
Date: Tue Feb 28 19:01:10 2017
New Revision: 296554

URL: http://llvm.org/viewvc/llvm-project?rev=296554=rev
Log:
[PS4] Set our default dialect to C++11. NFC for other targets.
Reapplies r296209 now that r296549 has fixed what really seems to be
the last problematic test.

Modified:
cfe/trunk/lib/Frontend/CompilerInvocation.cpp

Modified: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInvocation.cpp?rev=296554=296553=296554=diff
==
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp (original)
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp Tue Feb 28 19:01:10 2017
@@ -1582,7 +1582,11 @@ void CompilerInvocation::setLangDefaults
 case IK_PreprocessedCXX:
 case IK_ObjCXX:
 case IK_PreprocessedObjCXX:
-  LangStd = LangStandard::lang_gnucxx98;
+  // The PS4 uses C++11 as the default C++ standard.
+  if (T.isPS4())
+LangStd = LangStandard::lang_gnucxx11;
+  else
+LangStd = LangStandard::lang_gnucxx98;
   break;
 case IK_RenderScript:
   LangStd = LangStandard::lang_c99;


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

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

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

RE: r293457 - Tidy up codegen modules test & make it x86 specific since it relies on Itanium name manglings

[Robinson, Paul via cfe-commits]

Okay as is, then.  Thanks for the explanation.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Monday, January 30, 2017 9:32 AM
To: Robinson, Paul
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r293457 - Tidy up codegen modules test & make it x86 specific 
since it relies on Itanium name manglings

On reflection itanium wouldn't be sufficient/my comment wasn't sufficiently 
descriptive (I realized after I made this change that it would also fix the 
MacOS buildbot failure I was seeing & hadn't understood) - Darwin doesn't use 
comdats, for example, but is still an itanium ABI.

I could remove the checks for comdats - but I think they're beneficial/relevant.

Could refactor them to be optional somehow, but not sure it's worth it?

On Mon, Jan 30, 2017 at 9:28 AM Robinson, Paul 
> wrote:
Use %itanium_abi_triple instead?

> -Original Message-
> From: cfe-commits 
> [mailto:cfe-commits-boun...@lists.llvm.org]
>  On Behalf Of
> David Blaikie via cfe-commits
> Sent: Sunday, January 29, 2017 9:34 PM
> To: cfe-commits@lists.llvm.org
> Subject: r293457 - Tidy up codegen modules test & make it x86 specific
> since it relies on Itanium name manglings
>
> Author: dblaikie
> Date: Sun Jan 29 23:33:51 2017
> New Revision: 293457
>
> URL: http://llvm.org/viewvc/llvm-project?rev=293457=rev
> Log:
> Tidy up codegen modules test & make it x86 specific since it relies on
> Itanium name manglings
>
> Modified:
> cfe/trunk/test/Modules/codegen.test
>
> Modified: cfe/trunk/test/Modules/codegen.test
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/Modules/codegen.test?rev=293457=293456=293457
> =diff
> ==
> 
> --- cfe/trunk/test/Modules/codegen.test (original)
> +++ cfe/trunk/test/Modules/codegen.test Sun Jan 29 23:33:51 2017
> @@ -1,15 +1,16 @@
>  RUN: rm -rf %t
> +REQUIRES: x86-registered-target
>
> -RUN: %clang_cc1 -fmodules-codegen -x c++ -fmodules -emit-module -fmodule-
> name=foo %S/Inputs/codegen/foo.modulemap -o %t/foo.pcm
> -RUN: %clang_cc1 -fmodules-codegen -x c++ -fmodules -emit-module -fmodule-
> name=bar %S/Inputs/codegen/bar.modulemap -o %t/bar.pcm -fmodule-
> file=%t/foo.pcm
> +RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++ -
> fmodules -emit-module -fmodule-name=foo %S/Inputs/codegen/foo.modulemap -o
> %t/foo.pcm
> +RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++ -
> fmodules -emit-module -fmodule-name=bar %S/Inputs/codegen/bar.modulemap -o
> %t/bar.pcm -fmodule-file=%t/foo.pcm
>
> -RUN: %clang_cc1 -emit-llvm %t/foo.pcm -o - | FileCheck --check-prefix=FOO
> %s
> -RUN: %clang_cc1 -emit-llvm %t/bar.pcm -o - -fmodule-file=%t/foo.pcm |
> FileCheck --check-prefix=BAR-CMN --check-prefix=BAR %s
> -RUN: %clang_cc1 -fmodules -fmodule-file=%t/foo.pcm -fmodule-
> file=%t/bar.pcm %S/Inputs/codegen/use.cpp -emit-llvm -o - | FileCheck --
> check-prefix=USE-CMN --check-prefix=USE %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %t/foo.pcm |
> FileCheck --check-prefix=FOO %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %t/bar.pcm -
> fmodule-file=%t/foo.pcm | FileCheck --check-prefix=BAR-CMN --check-
> prefix=BAR %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -fmodules -
> fmodule-file=%t/foo.pcm -fmodule-file=%t/bar.pcm %S/Inputs/codegen/use.cpp
> | FileCheck --check-prefix=USE-CMN --check-prefix=USE %s
>
> -RUN: %clang_cc1 -O2 -disable-llvm-passes -emit-llvm %t/foo.pcm -o - |
> FileCheck --check-prefix=FOO %s
> -RUN: %clang_cc1 -O2 -disable-llvm-passes -emit-llvm %t/bar.pcm -o - -
> fmodule-file=%t/foo.pcm | FileCheck --check-prefix=BAR-CMN --check-
> prefix=BAR-OPT %s
> -RUN: %clang_cc1 -O2 -disable-llvm-passes -fmodules -fmodule-
> file=%t/foo.pcm -fmodule-file=%t/bar.pcm %S/Inputs/codegen/use.cpp -emit-
> llvm -o - | FileCheck --check-prefix=USE-CMN --check-prefix=USE-OPT %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-
> llvm-passes %t/foo.pcm | FileCheck --check-prefix=FOO %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-
> llvm-passes %t/bar.pcm -fmodule-file=%t/foo.pcm | FileCheck --check-
> prefix=BAR-CMN --check-prefix=BAR-OPT %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-
> llvm-passes -fmodules -fmodule-file=%t/foo.pcm -fmodule-file=%t/bar.pcm
> %S/Inputs/codegen/use.cpp | FileCheck --check-prefix=USE-CMN --check-
> prefix=USE-OPT %s
>
>  FOO-NOT: comdat
>  FOO: $_Z3foov = comdat any
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

RE: r293457 - Tidy up codegen modules test & make it x86 specific since it relies on Itanium name manglings

[Robinson, Paul via cfe-commits]

Use %itanium_abi_triple instead?

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> David Blaikie via cfe-commits
> Sent: Sunday, January 29, 2017 9:34 PM
> To: cfe-commits@lists.llvm.org
> Subject: r293457 - Tidy up codegen modules test & make it x86 specific
> since it relies on Itanium name manglings
> 
> Author: dblaikie
> Date: Sun Jan 29 23:33:51 2017
> New Revision: 293457
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=293457=rev
> Log:
> Tidy up codegen modules test & make it x86 specific since it relies on
> Itanium name manglings
> 
> Modified:
> cfe/trunk/test/Modules/codegen.test
> 
> Modified: cfe/trunk/test/Modules/codegen.test
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/Modules/codegen.test?rev=293457=293456=293457
> =diff
> ==
> 
> --- cfe/trunk/test/Modules/codegen.test (original)
> +++ cfe/trunk/test/Modules/codegen.test Sun Jan 29 23:33:51 2017
> @@ -1,15 +1,16 @@
>  RUN: rm -rf %t
> +REQUIRES: x86-registered-target
> 
> -RUN: %clang_cc1 -fmodules-codegen -x c++ -fmodules -emit-module -fmodule-
> name=foo %S/Inputs/codegen/foo.modulemap -o %t/foo.pcm
> -RUN: %clang_cc1 -fmodules-codegen -x c++ -fmodules -emit-module -fmodule-
> name=bar %S/Inputs/codegen/bar.modulemap -o %t/bar.pcm -fmodule-
> file=%t/foo.pcm
> +RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++ -
> fmodules -emit-module -fmodule-name=foo %S/Inputs/codegen/foo.modulemap -o
> %t/foo.pcm
> +RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++ -
> fmodules -emit-module -fmodule-name=bar %S/Inputs/codegen/bar.modulemap -o
> %t/bar.pcm -fmodule-file=%t/foo.pcm
> 
> -RUN: %clang_cc1 -emit-llvm %t/foo.pcm -o - | FileCheck --check-prefix=FOO
> %s
> -RUN: %clang_cc1 -emit-llvm %t/bar.pcm -o - -fmodule-file=%t/foo.pcm |
> FileCheck --check-prefix=BAR-CMN --check-prefix=BAR %s
> -RUN: %clang_cc1 -fmodules -fmodule-file=%t/foo.pcm -fmodule-
> file=%t/bar.pcm %S/Inputs/codegen/use.cpp -emit-llvm -o - | FileCheck --
> check-prefix=USE-CMN --check-prefix=USE %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %t/foo.pcm |
> FileCheck --check-prefix=FOO %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %t/bar.pcm -
> fmodule-file=%t/foo.pcm | FileCheck --check-prefix=BAR-CMN --check-
> prefix=BAR %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -fmodules -
> fmodule-file=%t/foo.pcm -fmodule-file=%t/bar.pcm %S/Inputs/codegen/use.cpp
> | FileCheck --check-prefix=USE-CMN --check-prefix=USE %s
> 
> -RUN: %clang_cc1 -O2 -disable-llvm-passes -emit-llvm %t/foo.pcm -o - |
> FileCheck --check-prefix=FOO %s
> -RUN: %clang_cc1 -O2 -disable-llvm-passes -emit-llvm %t/bar.pcm -o - -
> fmodule-file=%t/foo.pcm | FileCheck --check-prefix=BAR-CMN --check-
> prefix=BAR-OPT %s
> -RUN: %clang_cc1 -O2 -disable-llvm-passes -fmodules -fmodule-
> file=%t/foo.pcm -fmodule-file=%t/bar.pcm %S/Inputs/codegen/use.cpp -emit-
> llvm -o - | FileCheck --check-prefix=USE-CMN --check-prefix=USE-OPT %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-
> llvm-passes %t/foo.pcm | FileCheck --check-prefix=FOO %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-
> llvm-passes %t/bar.pcm -fmodule-file=%t/foo.pcm | FileCheck --check-
> prefix=BAR-CMN --check-prefix=BAR-OPT %s
> +RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - -O2 -disable-
> llvm-passes -fmodules -fmodule-file=%t/foo.pcm -fmodule-file=%t/bar.pcm
> %S/Inputs/codegen/use.cpp | FileCheck --check-prefix=USE-CMN --check-
> prefix=USE-OPT %s
> 
>  FOO-NOT: comdat
>  FOO: $_Z3foov = comdat any
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r292568 - [test] Remove an unwanted match for `UNSUPPORTED:`.

[Robinson, Paul via cfe-commits]

I'd call this a bug in lit; it shouldn't be matching partial tokens.
--paulr

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Greg Parker via cfe-commits
> Sent: Thursday, January 19, 2017 6:12 PM
> To: cfe-commits@lists.llvm.org
> Subject: r292568 - [test] Remove an unwanted match for `UNSUPPORTED:`.
> 
> Author: gparker
> Date: Thu Jan 19 20:12:22 2017
> New Revision: 292568
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=292568=rev
> Log:
> [test] Remove an unwanted match for `UNSUPPORTED:`.
> 
> Modified:
> cfe/trunk/test/Driver/embed-bitcode.c
> 
> Modified: cfe/trunk/test/Driver/embed-bitcode.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/embed-
> bitcode.c?rev=292568=292567=292568=diff
> ==
> 
> --- cfe/trunk/test/Driver/embed-bitcode.c (original)
> +++ cfe/trunk/test/Driver/embed-bitcode.c Thu Jan 19 20:12:22 2017
> @@ -51,5 +51,5 @@
>  // CHECK-NO-LINKER-NOT: -bitcode_bundle
> 
>  // RUN: %clang -target armv7-apple-darwin -miphoneos-version-min=5.0 %s -
> fembed-bitcode -### 2>&1 | \
> -// RUN:   FileCheck %s -check-prefix=CHECK-PLATFORM-UNSUPPORTED
> -// CHECK-PLATFORM-UNSUPPORTED: -fembed-bitcode is not supported on
> versions of iOS prior to 6.0
> +// RUN:   FileCheck %s -check-prefix=CHECK-PLATFORM-NOTSUPPORTED
> +// CHECK-PLATFORM-NOTSUPPORTED: -fembed-bitcode is not supported on
> versions of iOS prior to 6.0
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D28404: IRGen: Add optnone attribute on function during O0

[Robinson, Paul via cfe-commits]

(Re-add cfe-commits; otherwise same)

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Duncan P. N. Exon Smith via cfe-commits
> Sent: Monday, January 09, 2017 4:10 PM
> To: reviews+d28404+public+53e0f4655ef79...@reviews.llvm.org
> Cc: nhaeh...@gmail.com; wei.di...@amd.com; jholewin...@nvidia.com; Richard
> Smith; cfe-commits; Peter Collingbourne
> Subject: Re: [PATCH] D28404: IRGen: Add optnone attribute on function
> during O0
> 
> This seems like a massive rehash of a discussion Peter Collingbourne and I
> had about passing -O0 to the linker for -flto=full.  I had previously
> thought of LTO as "link time optimization", but in practice it's useful
> for (and required for correctness of some) non-optimization IR passes.
> 
> In other words, the basic question seems to be: "Should LTO support non-
> optimization use cases?"  I tend (now) to think it should -- having
> "optimization" in its name is an historical artifact -- because adding
> another way to run IR passes at link-time seems redundant.  Whereas, Paul,
> it seems like you disagree?

I am persuaded that there are non-optimization-based uses for clumps of
bitcode modules linked together.  (We could redefine the TLA if we like;
LTO = Link Time Operation?)

I am equally convinced that we have no good story for propagating a
variety of optimization- and codegen-related options to the top-level
LTO processor.  This is most especially true when different CUs might
reasonably have different options.  -O0 is the example at hand, but 
this problem seems to keep coming up and we keep hacking in ways to get 
the thing we think we need in the moment.

> 
> (Also, this discussion seems higher level than just the patch at hand...
> maybe llvm-dev would be more appropriate?)

I'd be fine with that.
--paulr
> 
> > On 2017-Jan-09, at 16:03, Paul Robinson via Phabricator
>  wrote:
> >
> > probinson added a comment.
> >
> > In https://reviews.llvm.org/D28404#640588, @mehdi_amini wrote:
> >
> >> Actually, as mentioned before, I could be fine with making `O0`
> incompatible with LTO, however security features like CFI (or other sort
> of whole-program analyses/instrumentations) requires LTO.
> >
> >
> > Well, "requires LTO" is overstating the case, AFAICT from the link you
> gave me.  Doesn't depend on //optimization// at all.  It depends on some
> interprocedural analyses given some particular scope/visibility boundary,
> which it is convenient to define as a set of linked bitcode modules, that
> by some happy chance is the same set of linked bitcode modules that LTO
> will operate on.
> >
> > If it's important to support combining a bitcode version of my-
> application with your-bitcode-library for this CFI or whatever, and you
> also want to let me have my-application be unoptimized while your-bitcode-
> library gets optimized, NOW we have a use-case.  (Maybe that's what you
> had in mind earlier, but for some reason I wasn't able to extract that out
> of any prior comments.  No matter.)
> >
> > I'm now thinking along the lines of a `-foptimize-off` flag (bikesheds
> welcome) which would set the default for the pragma to 'off'.  How is that
> different than what you wanted for `-O0`?  It is defined in terms of an
> existing pragma, which is WAY easier to explain and WAY easier to
> implement.  And, it still lets us say that `-c -O0 -flto` is a mistake, if
> that seems like a useful thing to say.
> >
> > Does that seem reasonable?  Fit your understanding of the needs?
> >
> >
> > https://reviews.llvm.org/D28404
> >
> >
> >
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r284793 - Remove 24 instances of 'REQUIRES: shell'

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Reid Kleckner via cfe-commits
> Sent: Thursday, October 20, 2016 4:12 PM
> To: cfe-commits@lists.llvm.org
> Subject: r284793 - Remove 24 instances of 'REQUIRES: shell'
> 
> Author: rnk
> Date: Thu Oct 20 18:11:45 2016
> New Revision: 284793
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=284793=rev
> Log:
> Remove 24 instances of 'REQUIRES: shell'
> 
> Tests fall into one of the following categories:
> 
> - The requirement was unnecessary
> 
> - Additional quoting was required for backslashes in paths (see "sed -e
>   's/\\//g'") in the sanitizer tests.
> 
> - OpenMP used 'REQUIRES: shell' as a proxy for the test failing on
>   Windows. Those tests fail there reliably, so use XFAIL instead.

Is there any expectation that they _should_ (eventually) work on Windows?
If not, seems like UNSUPPORTED would be clearer (and more efficient).
--paulr

> 
> I tried not to remove shell requirements that were added to suppress
> flaky test failures, but if I screwed up, we can add it back as needed.
> 
> Modified:
> cfe/trunk/test/Analysis/plist-html-macros.c
> cfe/trunk/test/CodeGen/address-safety-attr.cpp
> cfe/trunk/test/CodeGen/asan-globals.cpp
> cfe/trunk/test/CodeGen/sanitize-init-order.cpp
> cfe/trunk/test/CodeGen/sanitize-thread-attr.cpp
> cfe/trunk/test/CodeGen/ubsan-blacklist.c
> cfe/trunk/test/Driver/fsanitize-blacklist.c
> cfe/trunk/test/Driver/rewrite-map-in-diagnostics.c
> cfe/trunk/test/Modules/ModuleDebugInfo.cpp
> cfe/trunk/test/Modules/ModuleDebugInfo.m
> cfe/trunk/test/Modules/dependency-dump-dependent-module.m
> cfe/trunk/test/Modules/empty.modulemap
> cfe/trunk/test/Modules/explicit-build-extra-files.cpp
> cfe/trunk/test/Modules/prune.m
> cfe/trunk/test/Modules/signal.m
> cfe/trunk/test/OpenMP/task_firstprivate_codegen.cpp
> cfe/trunk/test/OpenMP/task_private_codegen.cpp
> cfe/trunk/test/OpenMP/taskloop_firstprivate_codegen.cpp
> cfe/trunk/test/OpenMP/taskloop_lastprivate_codegen.cpp
> cfe/trunk/test/OpenMP/taskloop_private_codegen.cpp
> cfe/trunk/test/OpenMP/taskloop_simd_firstprivate_codegen.cpp
> cfe/trunk/test/OpenMP/taskloop_simd_lastprivate_codegen.cpp
> cfe/trunk/test/OpenMP/taskloop_simd_private_codegen.cpp
> cfe/trunk/test/PCH/debug-info-pch-path.c
> 
> Modified: cfe/trunk/test/Analysis/plist-html-macros.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/plist-
> html-macros.c?rev=284793=284792=284793=diff
> ==
> 
> --- cfe/trunk/test/Analysis/plist-html-macros.c (original)
> +++ cfe/trunk/test/Analysis/plist-html-macros.c Thu Oct 20 18:11:45 2016
> @@ -1,12 +1,11 @@
> -// REQUIRES: shell
>  // RUN: %clang_cc1 -analyze -analyzer-checker=core -verify %s
>  // (sanity check)
> 
>  // RUN: rm -rf %t.dir
>  // RUN: mkdir -p %t.dir
>  // RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-
> output=plist-html -o %t.dir/index.plist %s
> -// RUN: ls %t.dir | grep \\.html | count 1
> -// RUN: grep \\.html %t.dir/index.plist | count 1
> +// RUN: ls %t.dir | grep '\.html' | count 1
> +// RUN: grep '\.html' %t.dir/index.plist | count 1
> 
>  // This tests two things: that the two calls to null_deref below are
> coalesced
>  // into a single bug by both the plist and HTML diagnostics, and that the
> plist
> 
> Modified: cfe/trunk/test/CodeGen/address-safety-attr.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/address-
> safety-attr.cpp?rev=284793=284792=284793=diff
> ==
> 
> --- cfe/trunk/test/CodeGen/address-safety-attr.cpp (original)
> +++ cfe/trunk/test/CodeGen/address-safety-attr.cpp Thu Oct 20 18:11:45
> 2016
> @@ -9,12 +9,11 @@ int DefinedInDifferentFile(int *a);
>  // RUN: echo "fun:*BlacklistedFunction*" > %t.func.blacklist
>  // RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin -emit-llvm -o -
> %s -include %t.extra-source.cpp -fsanitize=address -fsanitize-
> blacklist=%t.func.blacklist | FileCheck -check-prefix=BLFUNC %s
> 
> -// RUN: echo "src:%s" > %t.file.blacklist
> +// The blacklist file uses regexps, so escape backslashes, which are
> common in
> +// Windows paths.
> +// RUN: echo "src:%s" | sed -e 's/\\//g' > %t.file.blacklist
>  // RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin -emit-llvm -o -
> %s -include %t.extra-source.cpp -fsanitize=address -fsanitize-
> blacklist=%t.file.blacklist | FileCheck -check-prefix=BLFILE %s
> 
> -// FIXME: %t.file.blacklist is like
> "src:x:\path\to\clang\test\CodeGen\address-safety-attr.cpp"
> -// REQUIRES: shell
> -
>  // The sanitize_address attribute should be attached to functions
>  // when AddressSanitizer is enabled, unless no_sanitize_address attribute
>  // is present.
> 
> Modified: cfe/trunk/test/CodeGen/asan-globals.cpp

RE: r284416 - Driver/Darwin: Set the DWARF version based on the deployment target.

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Adrian Prantl via cfe-commits
> Sent: Monday, October 17, 2016 12:36 PM
> To: cfe-commits@lists.llvm.org
> Subject: r284416 - Driver/Darwin: Set the DWARF version based on the
> deployment target.
> 
> Author: adrian
> Date: Mon Oct 17 14:36:18 2016
> New Revision: 284416
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=284416=rev
> Log:
> Driver/Darwin: Set the DWARF version based on the deployment target.
> 
> System utilities such as atos only support DWARF 4 on OS X 10.11+ and
> iOS 9+. We thus want to enable DWARF 4 only if the deployment target
> has a recent enough operating system version and use DWARF 2 for older
> systems.
> 
> 
> 
> Modified:
> cfe/trunk/lib/Driver/ToolChains.cpp
> cfe/trunk/lib/Driver/ToolChains.h
> cfe/trunk/test/Driver/clang-g-opts.c
> cfe/trunk/test/Driver/debug-options.c
> 
> Modified: cfe/trunk/lib/Driver/ToolChains.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/Driver/ToolChains.cpp?rev=284416=284415=284416
> =diff
> ==
> 
> --- cfe/trunk/lib/Driver/ToolChains.cpp (original)
> +++ cfe/trunk/lib/Driver/ToolChains.cpp Mon Oct 17 14:36:18 2016
> @@ -289,6 +289,14 @@ void DarwinClang::AddLinkARCArgs(const A
>CmdArgs.push_back(Args.MakeArgString(P));
>  }
> 
> +unsigned DarwinClang::GetDefaultDwarfVersion() const {
> +  // Default to use DWARF 2 on OS X 10.10 / iOS 8 and lower.
> +  if ((isTargetMacOS() && isMacosxVersionLT(10, 11)) ||
> +  (isTargetIOSBased() && isIPhoneOSVersionLT(9)))
> +return 2;
> +  return 4;
> +}
> +
>  void MachO::AddLinkRuntimeLib(const ArgList , ArgStringList
> ,
>StringRef DarwinLibName, bool AlwaysLink,
>bool IsEmbedded, bool AddRPath) const {
> 
> Modified: cfe/trunk/lib/Driver/ToolChains.h
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/Driver/ToolChains.h?rev=284416=284415=284416
> iew=diff
> ==
> 
> --- cfe/trunk/lib/Driver/ToolChains.h (original)
> +++ cfe/trunk/lib/Driver/ToolChains.h Mon Oct 17 14:36:18 2016
> @@ -585,7 +585,7 @@ public:
>void AddLinkARCArgs(const llvm::opt::ArgList ,
>llvm::opt::ArgStringList ) const override;
> 
> -  unsigned GetDefaultDwarfVersion() const override { return 4; }
> +  unsigned GetDefaultDwarfVersion() const override;
>// Until dtrace (via CTF) and LLDB can deal with distributed debug
> info,
>// Darwin defaults to standalone/full debug info.
>bool GetDefaultStandaloneDebug() const override { return true; }
> 
> Modified: cfe/trunk/test/Driver/clang-g-opts.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/clang-g-
> opts.c?rev=284416=284415=284416=diff
> ==
> 
> --- cfe/trunk/test/Driver/clang-g-opts.c (original)
> +++ cfe/trunk/test/Driver/clang-g-opts.c Mon Oct 17 14:36:18 2016
> @@ -4,7 +4,7 @@
> 
>  // Assert that the toolchains which should default to a lower Dwarf
> version do so.
>  // RUN: %clang -### -S %s -g -target x86_64-apple-darwin 2>&1 \
> -// RUN: | FileCheck --check-prefix=CHECK-WITH-G-STANDALONE %s
> +// RUN: | FileCheck --check-prefix=CHECK-WITH-G-DWARF2 %s

This seems to weaken the test unnecessarily.
That is, "G-STANDALONE" should still work and checks more things.
--paulr

>  // RUN: %clang -### -S %s -g -target i686-pc-openbsd 2>&1 \
>  // RUN: | FileCheck --check-prefix=CHECK-WITH-G-DWARF2 %s
>  // RUN: %clang -### -S %s -g -target x86_64-pc-freebsd10.0 2>&1 \
> @@ -36,4 +36,4 @@
>  // CHECK-WITH-G-DWARF2: "-dwarf-version=2"
> 
>  // CHECK-WITH-G-STANDALONE: "-debug-info-kind=standalone"
> -// CHECK-WITH-G-STANDALONE: "-dwarf-version=4"
> +// CHECK-WITH-G-STANDALONE: "-dwarf-version=2"
> 
> Modified: cfe/trunk/test/Driver/debug-options.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/debug-
> options.c?rev=284416=284415=284416=diff
> ==
> 
> --- cfe/trunk/test/Driver/debug-options.c (original)
> +++ cfe/trunk/test/Driver/debug-options.c Mon Oct 17 14:36:18 2016
> @@ -19,17 +19,31 @@
>  // RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
> 
>  // RUN: %clang -### -c -g %s -target x86_64-apple-darwin 2>&1 \
> -// RUN: | FileCheck -check-prefix=G_DARWIN -check-
> prefix=G_LLDB %s
> -// RUN: %clang -### -c -g2 %s -target x86_64-apple-darwin 2>&1 \
> -// RUN: | FileCheck -check-prefix=G_DARWIN %s
> -// RUN: %clang -### -c -g3 %s -target x86_64-apple-darwin 2>&1 \
> -// RUN: | FileCheck -check-prefix=G_DARWIN %s
> -// RUN: %clang -### -c -ggdb %s -target x86_64-apple-darwin 2>&1 \
> -// RUN:   

RE: r284174 - Disable swiftcall test on windows: More brutal way to appease windows bots

[Robinson, Paul via cfe-commits]

Is there a bug to track this problem so it doesn't get lost?
--paulr

From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of Reid 
Kleckner via cfe-commits
Sent: Thursday, October 13, 2016 4:02 PM
To: Arnold Schwaighofer
Cc: cfe-commits
Subject: Re: r284174 - Disable swiftcall test on windows: More brutal way to 
appease windows bots

These kinds of crashes typically happen when you have something like a 
use-after-destroy of a temporary, like a misuse of Twine or 
std::initializer_list.

On Thu, Oct 13, 2016 at 3:47 PM, Arnold Schwaighofer via cfe-commits 
> wrote:
Author: arnolds
Date: Thu Oct 13 17:47:03 2016
New Revision: 284174

URL: http://llvm.org/viewvc/llvm-project?rev=284174=rev
Log:
Disable swiftcall test on windows: More brutal way to appease windows bots

The backtrace on the bot does not give me any indication what is wrong.
The test case interestingly passes in stage2 of the build.
I don't have a way of debugging this.

Disable the test on windows and hope if there is truly a bug in the code that
was causing we will eventually run into this on other platforms.

Modified:
cfe/trunk/test/CodeGen/64bit-swiftcall.c

Modified: cfe/trunk/test/CodeGen/64bit-swiftcall.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/64bit-swiftcall.c?rev=284174=284173=284174=diff
==
--- cfe/trunk/test/CodeGen/64bit-swiftcall.c (original)
+++ cfe/trunk/test/CodeGen/64bit-swiftcall.c Thu Oct 13 17:47:03 2016
@@ -3,6 +3,9 @@

 // REQUIRES: aarch64-registered-target,x86-registered-target

+// The union_het_vecint test case crashes on windows bot but only in stage1 
and not in stage2.
+// UNSUPPORTED: system-windows
+
 #define SWIFTCALL __attribute__((swiftcall))
 #define OUT __attribute__((swift_indirect_result))
 #define ERROR __attribute__((swift_error_result))


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

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

RE: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

[Robinson, Paul via cfe-commits]

I hadn't thought Clang wanted to be *quite* so knowledgeable about targets, and 
similarly not so tightly tied to byte-addressable targets.  But if both of 
those things are actually okay, then it's fine to set the alignment value here 
to what would be passed through to DWARF.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Monday, September 12, 2016 6:11 PM
To: Robinson, Paul; reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org; 
vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com; 
mehdi.am...@apple.com
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder 
only if aligment was forced


On Mon, Sep 12, 2016 at 6:01 PM Robinson, Paul 
> wrote:
The text in the committee draft is different (e.g., the exhortation about 
non-default alignment is gone), with an example to the effect that a value of 8 
means the entity's address is a multiple of 8 (not 2^8).  So, alignment is 
conceived in terms of address bits, whatever those represent (not always bytes).
Not sure I quite follow. OK, so in an octet addressable world (which LLVM is - 
there have been some attempts to support non-octet addressing, but I don't 
think any have been near to successful) then DW_AT_alignment is byte alignment 
(1 means there are no zero bits in the address, 2 means there's 1 trailing zero 
bit in the address, etc).
If Clang is being infested with more target knowledge, okay, but that means 
tolerating the weirder targets in these cases.  Dividing by CHAR_BITS makes an 
assumption that isn't necessarily correct.
Clang has the knowledge already - it knows the alignment of the types its 
allocating, etc. So I'm not sure what infestation you're referring to.

I've sort of lost track of what we're discussing here.

Essentially what I'm suggesting is that Clang should put whatever number is 
going to go in the DWARF, into the metadata. I don't believe the LLVM backends 
have greater knowledge than the frontend does in this domain - have I missed 
something there, are there examples where that could/would be true?

- David

--paulr
P.S. The committee is hoping to get a draft out for public comment Real Soon 
Now.
Looking forward to it :)

From: cfe-commits 
[mailto:cfe-commits-boun...@lists.llvm.org]
 On Behalf Of David Blaikie via cfe-commits
Sent: Monday, September 12, 2016 5:12 PM
To: 
reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org;
 vlesc...@accesssoftek.com; 
echri...@gmail.com; 
apra...@apple.com; 
mehdi.am...@apple.com
Cc: cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder 
only if aligment was forced


On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson 
> wrote:
probinson added a subscriber: probinson.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

dblaikie wrote:
> is max alignment the right thing here? Should it be min alignment?
> (is alignment in bits the desired thing across all of this too? It looked 
> like in the backend patch there was some division by CHAR_BITS, etc?)
I should think bits is the right choice here; seems more the province of the 
backend to convert it into the appropriate addressable units (commonly but not 
universally chars).

The alternative thinking is that we've a generally sense we want to make more 
of this type information opaque to LLVM - so I'm somewhat inclined to make the 
frontend do the work of choosing what to emit and the backend just being as 
simple as possible.

Hmm, seems like the DWARF spec details I can find: 
http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 don't really specify what 
the value of DW_AT_alignment is, it's sort of assumed, by the looks of it? I'm 
assuming it's bytes, the same as the byte_size attribute.





https://reviews.llvm.org/D24426

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

RE: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder only if aligment was forced

[Robinson, Paul via cfe-commits]

The text in the committee draft is different (e.g., the exhortation about 
non-default alignment is gone), with an example to the effect that a value of 8 
means the entity's address is a multiple of 8 (not 2^8).  So, alignment is 
conceived in terms of address bits, whatever those represent (not always bytes).

If Clang is being infested with more target knowledge, okay, but that means 
tolerating the weirder targets in these cases.  Dividing by CHAR_BITS makes an 
assumption that isn't necessarily correct.
--paulr
P.S. The committee is hoping to get a draft out for public comment Real Soon 
Now.

From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of 
David Blaikie via cfe-commits
Sent: Monday, September 12, 2016 5:12 PM
To: reviews+d24426+public+6ee6274d38fdf...@reviews.llvm.org; 
vlesc...@accesssoftek.com; echri...@gmail.com; apra...@apple.com; 
mehdi.am...@apple.com
Cc: cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D24426: DebugInfo: Pass non-zero alignment to DIBuilder 
only if aligment was forced


On Mon, Sep 12, 2016 at 5:00 PM Paul Robinson 
> wrote:
probinson added a subscriber: probinson.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3691
@@ -3635,1 +3690,3 @@
+  if (D->hasAttr())
+AlignInBits = D->getMaxAlignment();
   StringRef DeclName, LinkageName;

dblaikie wrote:
> is max alignment the right thing here? Should it be min alignment?
> (is alignment in bits the desired thing across all of this too? It looked 
> like in the backend patch there was some division by CHAR_BITS, etc?)
I should think bits is the right choice here; seems more the province of the 
backend to convert it into the appropriate addressable units (commonly but not 
universally chars).

The alternative thinking is that we've a generally sense we want to make more 
of this type information opaque to LLVM - so I'm somewhat inclined to make the 
frontend do the work of choosing what to emit and the backend just being as 
simple as possible.

Hmm, seems like the DWARF spec details I can find: 
http://www.dwarfstd.org/ShowIssue.php?issue=140528.1 don't really specify what 
the value of DW_AT_alignment is, it's sort of assumed, by the looks of it? I'm 
assuming it's bytes, the same as the byte_size attribute.





https://reviews.llvm.org/D24426


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

RE: r280057 - Combine two FileCheck patterns to prevent overzealous matching of .*

[Robinson, Paul via cfe-commits]

The case that failed on the build not was something like

-o blah.o -x c++ -o system /.../llvm.org/...

... where the first line of the old pattern matched up to the .o in 
llvm.org, causing the second line to fail to match.

That suggests the .* behavior is 'greedy' (matching as much as possible rather 
than as little as possible) which is probably not the behavior we want (clearly 
not the behavior the test author expected).  I'll file a bug.
--paulr

From: meta...@gmail.com [mailto:meta...@gmail.com] On Behalf Of Richard Smith
Sent: Tuesday, August 30, 2016 9:55 AM
To: Robinson, Paul
Cc: cfe-commits
Subject: RE: r280057 - Combine two FileCheck patterns to prevent overzealous 
matching of .*

On 30 Aug 2016 9:45 am, "Robinson, Paul" 
> wrote:


> -Original Message-
> From: cfe-commits 
> [mailto:cfe-commits-boun...@lists.llvm.org]
>  On Behalf Of
> Richard Smith via cfe-commits
> Sent: Monday, August 29, 2016 10:15 PM
> To: cfe-commits@lists.llvm.org
> Subject: r280057 - Combine two FileCheck patterns to prevent overzealous
> matching of .*
>
> Author: rsmith
> Date: Tue Aug 30 00:14:38 2016
> New Revision: 280057
>
> URL: http://llvm.org/viewvc/llvm-project?rev=280057=rev
> Log:
> Combine two FileCheck patterns to prevent overzealous matching of .*
>
> Modified:
> cfe/trunk/test/Driver/modules-ts.cpp
>
> Modified: cfe/trunk/test/Driver/modules-ts.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/modules-
> ts.cpp?rev=280057=280056=280057=diff
> ==
> 
> --- cfe/trunk/test/Driver/modules-ts.cpp (original)
> +++ cfe/trunk/test/Driver/modules-ts.cpp Tue Aug 30 00:14:38 2016
> @@ -23,8 +23,7 @@
>  // CHECK-USE: -cc1
>  // CHECK-USE-SAME: -emit-obj
>  // CHECK-USE-SAME: -fmodule-file={{.*}}.pcm
> -// CHECK-USE-SAME: -o {{.*}}.o
> -// CHECK-USE-SAME: -x c++
> +// CHECK-USE-SAME: -o {{.*}}.o {{.*}}-x c++
Sorry--how are these not doing the same thing?
That is, what input will incorrectly fail with the old checks?

The case that failed on the build not was something like

-o blah.o -x c++ -o system /.../llvm.org/...

... where the first line of the old pattern matched up to the .o in 
llvm.org, causing the second line to fail to match.

Thanks,
--paulr

>  // CHECK-USE-SAME: modules-ts.cpp
>
>  // Check combining precompile and compile steps works.
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

RE: r280057 - Combine two FileCheck patterns to prevent overzealous matching of .*

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Richard Smith via cfe-commits
> Sent: Monday, August 29, 2016 10:15 PM
> To: cfe-commits@lists.llvm.org
> Subject: r280057 - Combine two FileCheck patterns to prevent overzealous
> matching of .*
> 
> Author: rsmith
> Date: Tue Aug 30 00:14:38 2016
> New Revision: 280057
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=280057=rev
> Log:
> Combine two FileCheck patterns to prevent overzealous matching of .*
> 
> Modified:
> cfe/trunk/test/Driver/modules-ts.cpp
> 
> Modified: cfe/trunk/test/Driver/modules-ts.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/modules-
> ts.cpp?rev=280057=280056=280057=diff
> ==
> 
> --- cfe/trunk/test/Driver/modules-ts.cpp (original)
> +++ cfe/trunk/test/Driver/modules-ts.cpp Tue Aug 30 00:14:38 2016
> @@ -23,8 +23,7 @@
>  // CHECK-USE: -cc1
>  // CHECK-USE-SAME: -emit-obj
>  // CHECK-USE-SAME: -fmodule-file={{.*}}.pcm
> -// CHECK-USE-SAME: -o {{.*}}.o
> -// CHECK-USE-SAME: -x c++
> +// CHECK-USE-SAME: -o {{.*}}.o {{.*}}-x c++

Sorry--how are these not doing the same thing?
That is, what input will incorrectly fail with the old checks?
Thanks,
--paulr

>  // CHECK-USE-SAME: modules-ts.cpp
> 
>  // Check combining precompile and compile steps works.
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r278710 - Replace an obsolete company name.

[Robinson, Paul via cfe-commits]

I had tried doing that in the Users.html page when I first added an entry 
there, and it caused problems.  So, I'd rather not embed a unicode character 
directly into the file, and I'd rather use a semi-intelligible substitution 
name than some raw hex value.  The intent is clearer.
--paulr

From: Sean Silva [mailto:chisophu...@gmail.com]
Sent: Monday, August 15, 2016 1:09 PM
To: Robinson, Paul
Cc: cfe-commits
Subject: Re: r278710 - Replace an obsolete company name.

You may also want to just try using the unicode character (not that it really 
matters that much though).

On Mon, Aug 15, 2016 at 11:45 AM, Paul Robinson via cfe-commits 
> wrote:
Author: probinson
Date: Mon Aug 15 13:45:52 2016
New Revision: 278710

URL: http://llvm.org/viewvc/llvm-project?rev=278710=rev
Log:
Replace an obsolete company name.

Modified:
cfe/trunk/docs/UsersManual.rst

Modified: cfe/trunk/docs/UsersManual.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.rst?rev=278710=278709=278710=diff
==
--- cfe/trunk/docs/UsersManual.rst (original)
+++ cfe/trunk/docs/UsersManual.rst Mon Aug 15 13:45:52 2016
@@ -2,6 +2,8 @@
 Clang Compiler User's Manual
 

+.. include:: 
+
 .. contents::
:local:

@@ -1650,7 +1652,7 @@ features. You can "tune" the debug info

 .. option:: -ggdb, -glldb, -gsce

-  Tune the debug info for the ``gdb``, ``lldb``, or Sony Computer Entertainment
+  Tune the debug info for the ``gdb``, ``lldb``, or Sony PlayStation\ |reg|
   debugger, respectively. Each of these options implies **-g**. (Therefore, if
   you want both **-gline-tables-only** and debugger tuning, the tuning option
   must come first.)


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

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

RE: r276102 - [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using generic IR

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Simon Pilgrim via cfe-commits
> Sent: Wednesday, July 20, 2016 3:18 AM
> To: cfe-commits@lists.llvm.org
> Subject: r276102 - [X86][SSE] Reimplement SSE fp2si conversion intrinsics
> instead of using generic IR
> 
> Author: rksimon
> Date: Wed Jul 20 05:18:01 2016
> New Revision: 276102
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=276102=rev
> Log:
> [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using
> generic IR
> 
> D20859 and D20860 attempted to replace the SSE (V)CVTTPS2DQ and VCVTTPD2DQ
> truncating conversions with generic IR instead.
> 
> It turns out that the behaviour of these intrinsics is different enough
> from generic IR that this will cause problems, INF/NAN/out of range values
> are guaranteed to result in a 0x8000 value - which plays havoc with
> constant folding which converts them to either zero or UNDEF. This is also
> an issue with the scalar implementations (which were already generic IR
> and what I was trying to match).

Are the problems enough that this should be merged to the 3.9 release branch?
--paulr

> 
> This patch changes both scalar and packed versions back to using x86-
> specific builtins.
> 
> It also deals with the other scalar conversion cases that are runtime
> rounding mode dependent and can have similar issues with constant folding.
> 
> Differential Revision: https://reviews.llvm.org/D22105
> 
> Modified:
> cfe/trunk/include/clang/Basic/BuiltinsX86.def
> cfe/trunk/lib/Headers/avxintrin.h
> cfe/trunk/lib/Headers/emmintrin.h
> cfe/trunk/lib/Headers/xmmintrin.h
> cfe/trunk/test/CodeGen/avx-builtins.c
> cfe/trunk/test/CodeGen/builtins-x86.c
> cfe/trunk/test/CodeGen/sse-builtins.c
> cfe/trunk/test/CodeGen/sse2-builtins.c
> 
> Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/include/clang/Basic/BuiltinsX86.def?rev=276102=276101
> =276102=diff
> ==
> 
> --- cfe/trunk/include/clang/Basic/BuiltinsX86.def (original)
> +++ cfe/trunk/include/clang/Basic/BuiltinsX86.def Wed Jul 20 05:18:01 2016
> @@ -303,7 +303,9 @@ TARGET_BUILTIN(__builtin_ia32_pabsd128,
>  TARGET_BUILTIN(__builtin_ia32_ldmxcsr, "vUi", "", "sse")
>  TARGET_BUILTIN(__builtin_ia32_stmxcsr, "Ui", "", "sse")
>  TARGET_BUILTIN(__builtin_ia32_cvtss2si, "iV4f", "", "sse")
> +TARGET_BUILTIN(__builtin_ia32_cvttss2si, "iV4f", "", "sse")
>  TARGET_BUILTIN(__builtin_ia32_cvtss2si64, "LLiV4f", "", "sse")
> +TARGET_BUILTIN(__builtin_ia32_cvttss2si64, "LLiV4f", "", "sse")
>  TARGET_BUILTIN(__builtin_ia32_storehps, "vV2i*V4f", "", "sse")
>  TARGET_BUILTIN(__builtin_ia32_storelps, "vV2i*V4f", "", "sse")
>  TARGET_BUILTIN(__builtin_ia32_movmskps, "iV4f", "", "sse")
> @@ -328,8 +330,12 @@ TARGET_BUILTIN(__builtin_ia32_cvtpd2dq,
>  TARGET_BUILTIN(__builtin_ia32_cvtpd2ps, "V4fV2d", "", "sse2")
>  TARGET_BUILTIN(__builtin_ia32_cvttpd2dq, "V4iV2d", "", "sse2")
>  TARGET_BUILTIN(__builtin_ia32_cvtsd2si, "iV2d", "", "sse2")
> +TARGET_BUILTIN(__builtin_ia32_cvttsd2si, "iV2d", "", "sse2")
>  TARGET_BUILTIN(__builtin_ia32_cvtsd2si64, "LLiV2d", "", "sse2")
> +TARGET_BUILTIN(__builtin_ia32_cvttsd2si64, "LLiV2d", "", "sse2")
> +TARGET_BUILTIN(__builtin_ia32_cvtsd2ss, "V4fV4fV2d", "", "sse2")
>  TARGET_BUILTIN(__builtin_ia32_cvtps2dq, "V4iV4f", "", "sse2")
> +TARGET_BUILTIN(__builtin_ia32_cvttps2dq, "V4iV4f", "", "sse2")
>  TARGET_BUILTIN(__builtin_ia32_clflush, "vvC*", "", "sse2")
>  TARGET_BUILTIN(__builtin_ia32_lfence, "v", "", "sse2")
>  TARGET_BUILTIN(__builtin_ia32_mfence, "v", "", "sse2")
> @@ -455,7 +461,9 @@ TARGET_BUILTIN(__builtin_ia32_cmpss, "V4
>  TARGET_BUILTIN(__builtin_ia32_cvtdq2ps256, "V8fV8i", "", "avx")
>  TARGET_BUILTIN(__builtin_ia32_cvtpd2ps256, "V4fV4d", "", "avx")
>  TARGET_BUILTIN(__builtin_ia32_cvtps2dq256, "V8iV8f", "", "avx")
> +TARGET_BUILTIN(__builtin_ia32_cvttpd2dq256, "V4iV4d", "", "avx")
>  TARGET_BUILTIN(__builtin_ia32_cvtpd2dq256, "V4iV4d", "", "avx")
> +TARGET_BUILTIN(__builtin_ia32_cvttps2dq256, "V8iV8f", "", "avx")
>  TARGET_BUILTIN(__builtin_ia32_vperm2f128_pd256, "V4dV4dV4dIc", "", "avx")
>  TARGET_BUILTIN(__builtin_ia32_vperm2f128_ps256, "V8fV8fV8fIc", "", "avx")
>  TARGET_BUILTIN(__builtin_ia32_vperm2f128_si256, "V8iV8iV8iIc", "", "avx")
> 
> Modified: cfe/trunk/lib/Headers/avxintrin.h
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/Headers/avxintrin.h?rev=276102=276101=276102
> iew=diff
> ==
> 
> --- cfe/trunk/lib/Headers/avxintrin.h (original)
> +++ cfe/trunk/lib/Headers/avxintrin.h Wed Jul 20 05:18:01 2016
> @@ -2117,7 +2117,7 @@ _mm256_cvtps_pd(__m128 __a)
>  static __inline __m128i __DEFAULT_FN_ATTRS
>  _mm256_cvttpd_epi32(__m256d __a)
>  {
> -  return (__m128i)__builtin_convertvector((__v4df) __a, 

RE: [PATCH] D22463: [RFC] Moving to GitHub Proposal: NOT DECISION!

[Robinson, Paul via cfe-commits]

> > I think we could emulate any pre-commit hook we like via GitHub
> > WebHooks by having two repositories: llvm and llvm-staging (say).
> >
> > People push to llvm-staging, which notifies some LLVM server we own.
> > That does basic sanity checks and pushes to llvm proper if passed.
> 
> I think that would be terrible in practice, for instance how do you handle
> situations like:
> 
> 1) Dev A push commit A
> 2) Dev B push commit B that changes some lines close to the one changed by
> commit A
> 3) sanity check fails on commit A, but llvm-staging contains A and B and
> can’t get rid of A easily because B would not apply without A.
> 
> At this point Dev B gets an email (or other mechanism, I don’t know what
> you imagined) telling that his changed are rejected for no good reason.
> 
> Also reverting to a state "before A” on llvm-staging would mean that that
> the history would be rewritten and everyone that pulled/fetched in the
> meantime would suffer .
> 
> If we want to go towards pre-check using staging, I believe we should work
> with pull-request (we’d still have the issue of conflicting PR, but I
> don’t believe it’d be that bad in practice).
> That’d be welcome, but that’d also be a whole other story to setup and
> maintain!
> 
> —
> Mehdi

Hear hear.  The schemes to handle this that I'm aware of do look more like
pull requests.  You submit your change to the pre-qualification queue.
If it rebases cleanly and passes pre-qual, your change becomes the new HEAD.
If it doesn't rebase cleanly or fails pre-qual, your change gets bounced.

A pull-request-like model also helps avoid the rebase-build-test-push(fail)
loop that you can get into with a very active git-based project.  This is
a mechanical task best suited to automation rather than wasting valuable
developer time on it.  But I suspect GitHub does not have anything like
this OOB so it would be an enhancement for a later time.
--paulr

P.S. At Sony we are building a system with a "staging" step but it's not
for our own work... more of a "flood control" dam.  :-)

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

RE: [PATCH] D22096: [OpenMP] Sema and parsing for 'target parallel for simd' pragma

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: Alexey Bataev [mailto:a.bat...@hotmail.com]
> Sent: Thursday, July 14, 2016 7:51 PM
> To: reviews+d22096+public+4c00789034d62...@reviews.llvm.org
> Cc: cfe-commits@lists.llvm.org; kkw...@gmail.com; cber...@us.ibm.com;
> sfan...@us.ibm.com; hfin...@anl.gov; acja...@us.ibm.com; Robinson, Paul
> Subject: Re: [PATCH] D22096: [OpenMP] Sema and parsing for 'target
> parallel for simd' pragma
> 
> Hi Paul,
> Could you provide a little bit more info about diagnostic you see? Maybe
> the tests just need to be fixed

That would be fine too.  Log of the two failing tests attached.
Thanks,
--paulr

> 
> Best regards,
> Alexey Bataev
> =
> Software Engineer
> Intel Compiler Team
> 
> 15.07.2016 2:09, Paul Robinson пишет:
> > probinson added a subscriber: probinson.
> > probinson added a comment.
> >
> > I'm seeing a different set of diagnostics in two of these tests, because
> we default to C+11, which makes them fail for us.  Ideally you'd
> conditionalize the tests on the value of `__cplusplus` (like Charles Li
> has been doing for a whole lot of the Clang tests).  If that's
> inconvenient for you right now, would you mind if I added `-std=c++03` to
> the following tests?  It's just these two that need it:
> >
> > target_parallel_for_simd_collapse_messages.cpp
> > target_parallel_for_simd_ordered_messages.cpp
> >
> >
> > Repository:
> >rL LLVM
> >
> > https://reviews.llvm.org/D22096
> >
> >
> >

FAIL: Clang :: OpenMP/target_parallel_for_simd_collapse_messages.cpp (307 of 
430)
 TEST 'Clang :: 
OpenMP/target_parallel_for_simd_collapse_messages.cpp' FAILED 

Script:
--
/home/probinson/ssd/projects/staging/o/build-gcc-ps4-sce-staging-test-ninja/./bin/clang
 -cc1 -internal-isystem 
/home/probinson/ssd/projects/staging/o/build-gcc-ps4-sce-staging-test-ninja/bin/../lib/clang/include
 -nostdsysteminc -verify -fopenmp 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
--
Exit Code: 1

Command Output (stderr):
--
error: 'error' diagnostics expected but not seen: 
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 40 (directive at 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp:39):
 expression is not an integral constant expression
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 40 (directive at 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp:39):
 expression is not an integral constant expression
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 72 (directive at 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp:71):
 expression is not an integral constant expression
error: 'error' diagnostics seen but not expected: 
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 72: integral constant expression must have integral or unscoped 
enumeration type, not 'char *'
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 40: integral constant expression must have integral or unscoped 
enumeration type, not 'char *'
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 40: integral constant expression must have integral or unscoped 
enumeration type, not 'char *'
error: 'note' diagnostics seen but not expected: 
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 62: non-constexpr function 'foobool' cannot be used in a constant 
expression
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 6: declared here
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 67: non-constexpr function 'foobool' cannot be used in a constant 
expression
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 6: declared here
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 35: non-constexpr function 'foobool' cannot be used in a constant 
expression
  File 
/home/probinson/ssd/projects/staging/o/llvm/tools/clang/test/OpenMP/target_parallel_for_simd_collapse_messages.cpp
 Line 6: declared here
  File 

RE: r275040 - [CodeGen] Treat imported static local variables as declarations

[Robinson, Paul via cfe-commits]

I was asking for the debug info to describe the entity as a declaration, rather 
than a definition.
Instead you eliminated the debug-info description entirely.  These are pretty 
different things.
--paulr

From: David Majnemer [mailto:david.majne...@gmail.com]
Sent: Monday, July 11, 2016 12:27 PM
To: Robinson, Paul
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r275040 - [CodeGen] Treat imported static local variables as 
declarations



On Mon, Jul 11, 2016 at 11:45 AM, Robinson, Paul 
> wrote:
It was not particularly obvious that by "static local variables" you actually 
meant "template static data members."
Now I can tell that this is not addressing what I was asking about in the 
comment on r274986.

I'm not sure I understand.  How is this not addressing what you are asking for? 
 We will no longer emit a DIGlobalVariable for the imported static data member.

--paulr

From: David Majnemer 
[mailto:david.majne...@gmail.com]
Sent: Monday, July 11, 2016 9:53 AM
To: Robinson, Paul
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r275040 - [CodeGen] Treat imported static local variables as 
declarations



On Mon, Jul 11, 2016 at 9:48 AM, Robinson, Paul 
> wrote:
This changes the IR but not the debug-info metadata?
--paulr

The net effect is that the debug-info metadata is not generated for such static 
members.


> -Original Message-
> From: cfe-commits 
> [mailto:cfe-commits-boun...@lists.llvm.org]
>  On Behalf Of
> David Majnemer via cfe-commits
> Sent: Sunday, July 10, 2016 9:28 PM
> To: cfe-commits@lists.llvm.org
> Subject: r275040 - [CodeGen] Treat imported static local variables as
> declarations
>
> Author: majnemer
> Date: Sun Jul 10 23:28:21 2016
> New Revision: 275040
>
> URL: http://llvm.org/viewvc/llvm-project?rev=275040=rev
> Log:
> [CodeGen] Treat imported static local variables as declarations
>
> Imported variables cannot really be definitions for the purposes of
> IR generation.
>
> Modified:
> cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
> cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> cfe/trunk/test/CodeGenCXX/dllimport.cpp
>
> Modified: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/CodeGen/CGDeclCXX.cpp?rev=275040=275039=275040
> =diff
> ==
> 
> --- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp Sun Jul 10 23:28:21 2016
> @@ -323,10 +323,6 @@ CodeGenModule::EmitCXXGlobalVarDeclInitF
> D->hasAttr()))
>  return;
>
> -  // DLL imported variables will be initialized by the export side.
> -  if (D->hasAttr())
> -return;
> -
>// Check if we've already initialized this decl.
>auto I = DelayedCXXInitPosition.find(D);
>if (I != DelayedCXXInitPosition.end() && I->second == ~0U)
>
> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=275040=275039=27
> 5040=diff
> ==
> 
> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Sun Jul 10 23:28:21 2016
> @@ -2851,6 +2851,10 @@ static void ReplaceUsesOfNonProtoTypeWit
>  }
>
>  void CodeGenModule::HandleCXXStaticMemberVarInstantiation(VarDecl *VD) {
> +  auto DK = VD->isThisDeclarationADefinition();
> +  if (DK == VarDecl::Definition && VD->hasAttr())
> +return;
> +
>TemplateSpecializationKind TSK = VD->getTemplateSpecializationKind();
>// If we have a definition, this might be a deferred decl. If the
>// instantiation is explicit, make sure we emit it at the end.
>
> Modified: cfe/trunk/test/CodeGenCXX/dllimport.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/CodeGenCXX/dllimport.cpp?rev=275040=275039=27
> 5040=diff
> ==
> 
> --- cfe/trunk/test/CodeGenCXX/dllimport.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/dllimport.cpp Sun Jul 10 23:28:21 2016
> @@ -676,7 +676,7 @@ namespace ClassTemplateStaticDef {
>  static int x;
>};
>template  int S::x;
> -  // MSC-DAG: @"\01?x@?$S@H@ClassTemplateStaticDef@@2HA" =
> available_externally dllimport global i32 0
> +  // MSC-DAG: @"\01?x@?$S@H@ClassTemplateStaticDef@@2HA" = external
> dllimport global i32
>int f() { return S::x; }
>
>// Partial class template specialization static field:
> @@ -685,7 +685,7 @@ namespace ClassTemplateStaticDef {
>  static int x;
>};
>template  int T::x;
> -  // GNU-DAG: @_ZN22ClassTemplateStaticDef1TIPvE1xE =
> available_externally dllimport global i32 0
> +  // 

RE: r275040 - [CodeGen] Treat imported static local variables as declarations

[Robinson, Paul via cfe-commits]

This changes the IR but not the debug-info metadata?
--paulr

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> David Majnemer via cfe-commits
> Sent: Sunday, July 10, 2016 9:28 PM
> To: cfe-commits@lists.llvm.org
> Subject: r275040 - [CodeGen] Treat imported static local variables as
> declarations
> 
> Author: majnemer
> Date: Sun Jul 10 23:28:21 2016
> New Revision: 275040
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=275040=rev
> Log:
> [CodeGen] Treat imported static local variables as declarations
> 
> Imported variables cannot really be definitions for the purposes of
> IR generation.
> 
> Modified:
> cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
> cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> cfe/trunk/test/CodeGenCXX/dllimport.cpp
> 
> Modified: cfe/trunk/lib/CodeGen/CGDeclCXX.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/CodeGen/CGDeclCXX.cpp?rev=275040=275039=275040
> =diff
> ==
> 
> --- cfe/trunk/lib/CodeGen/CGDeclCXX.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDeclCXX.cpp Sun Jul 10 23:28:21 2016
> @@ -323,10 +323,6 @@ CodeGenModule::EmitCXXGlobalVarDeclInitF
> D->hasAttr()))
>  return;
> 
> -  // DLL imported variables will be initialized by the export side.
> -  if (D->hasAttr())
> -return;
> -
>// Check if we've already initialized this decl.
>auto I = DelayedCXXInitPosition.find(D);
>if (I != DelayedCXXInitPosition.end() && I->second == ~0U)
> 
> Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=275040=275039=27
> 5040=diff
> ==
> 
> --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Sun Jul 10 23:28:21 2016
> @@ -2851,6 +2851,10 @@ static void ReplaceUsesOfNonProtoTypeWit
>  }
> 
>  void CodeGenModule::HandleCXXStaticMemberVarInstantiation(VarDecl *VD) {
> +  auto DK = VD->isThisDeclarationADefinition();
> +  if (DK == VarDecl::Definition && VD->hasAttr())
> +return;
> +
>TemplateSpecializationKind TSK = VD->getTemplateSpecializationKind();
>// If we have a definition, this might be a deferred decl. If the
>// instantiation is explicit, make sure we emit it at the end.
> 
> Modified: cfe/trunk/test/CodeGenCXX/dllimport.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/CodeGenCXX/dllimport.cpp?rev=275040=275039=27
> 5040=diff
> ==
> 
> --- cfe/trunk/test/CodeGenCXX/dllimport.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/dllimport.cpp Sun Jul 10 23:28:21 2016
> @@ -676,7 +676,7 @@ namespace ClassTemplateStaticDef {
>  static int x;
>};
>template  int S::x;
> -  // MSC-DAG: @"\01?x@?$S@H@ClassTemplateStaticDef@@2HA" =
> available_externally dllimport global i32 0
> +  // MSC-DAG: @"\01?x@?$S@H@ClassTemplateStaticDef@@2HA" = external
> dllimport global i32
>int f() { return S::x; }
> 
>// Partial class template specialization static field:
> @@ -685,7 +685,7 @@ namespace ClassTemplateStaticDef {
>  static int x;
>};
>template  int T::x;
> -  // GNU-DAG: @_ZN22ClassTemplateStaticDef1TIPvE1xE =
> available_externally dllimport global i32 0
> +  // GNU-DAG: @_ZN22ClassTemplateStaticDef1TIPvE1xE = external dllimport
> global i32
>int g() { return T::x; }
>  }
> 
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r274084 - Revert "[PS4] Tighten up a test (noticed in passing)"

[Robinson, Paul via cfe-commits]

With some digging I worked out at least part of the history.  It's not that we 
care about PATH per se; the test is making sure that even if you don't have a 
linker in the package, there's still a file with the right name for clang to 
find in a place that it would (eventually) look.  If you *do* have a linker in 
the package, clang should find it and still construct the command line 
correctly.
An upstream lit test can't arrange a co-located linker (without extraordinary 
measures such as you suggest, which I think we don't want); we should have an 
internal regression test for that case, but apparently do not.

The *actual bug* is the missing .exe extension.  This appears to be a 
regression, looking at the original (internal) bug. You were correct to remove 
the directory part of the path check, but not to remove the .exe part, as that 
masked the regression.
You should be able to drop 'orbis-ld.exe' in the directory with clang and have 
clang find it correctly.  The same is not true if you drop 'orbis-ld' into that 
directory.
--paulr

From: Sean Silva [mailto:chisophu...@gmail.com]
Sent: Friday, July 01, 2016 12:09 AM
To: Robinson, Paul
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r274084 - Revert "[PS4] Tighten up a test (noticed in passing)"



On Thu, Jun 30, 2016 at 5:30 PM, Robinson, Paul 
> wrote:
Okay, that is peculiar.  But I can repro it.  If I put either orbis-ld or 
orbis-ld.exe co-located with clang.exe, it builds a command line without the 
.exe suffix (but using the directory where clang.exe lives).

Yeah, as I described in the commit message of r262285 it is due to an 
extra-high-priority lookup done in Driver::GetProgramPath that overrides the 
PATH lookup. In fact, in the production PS4 SDK the PATH lookup purportedly 
tested in this test *never occurs* since these "look in the the directory 
containing clang" lookups always find orbis-ld first (since it is always(?) 
right beside clang).

So this test probably needs to be changed to copy the clang binary into a 
directory, put an orbis-ld.exe there, and then verify that clang finds that 
orbis-ld. There is little point to testing PATH lookups since AFAIK we don't 
really advertise to licensees that they should mess with the contents of the 
sdk dir (i.e. to cause clang to not be beside orbis-ld and instead find 
orbis-ld through a PATH lookup).

(actually, copying the clang binary is not ideal since that binary may be quite 
large)


I do think a bug report would have been appropriate, rather than just munging 
the test…
As it is (with your mods) the test is not checking what we want it to check.  
I'll write an internal bug for this.

Thanks. To be completely honest I had not noticed that due to the prefix 
property it wouldn't quite check the right thing.

-- Sean Silva

--paulr

From: Sean Silva [mailto:chisophu...@gmail.com]
Sent: Thursday, June 30, 2016 4:03 PM
To: Robinson, Paul
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r274084 - Revert "[PS4] Tighten up a test (noticed in passing)"



On Thu, Jun 30, 2016 at 3:52 PM, Robinson, Paul 
> wrote:


> -Original Message-
> From: cfe-commits 
> [mailto:cfe-commits-boun...@lists.llvm.org]
>  On Behalf Of
> Sean Silva via cfe-commits
> Sent: Tuesday, June 28, 2016 5:29 PM
> To: cfe-commits@lists.llvm.org
> Subject: r274084 - Revert "[PS4] Tighten up a test (noticed in passing)"
>
> Author: silvas
> Date: Tue Jun 28 19:29:23 2016
> New Revision: 274084
>
> URL: http://llvm.org/viewvc/llvm-project?rev=274084=rev
> Log:
> Revert "[PS4] Tighten up a test (noticed in passing)"
>
> This reverts commit r269709.
>
> r262285 changed this deliberately so that the test would not be
> sensitive to which binaries are in the same directory as clang.
> See the commit message of that commit for more background.

Okay, but the point of the test is to match a "file.exe" instead
of just "file". See commentary at the top of the test.
Also "orbis-ld" is a prefix of "orbis-ld.gold" and so matching
just the former doesn't verify we're looking for the right one.

I understand taking out the path part of the check in r262285 but
if you named your test linker "orbis-ld.exe" instead of "orbis-ld"
then the test would pass with r269709, right?

Unfortunately not.

-- Sean Silva

  If that's all this
is about, please undo this revert and use the standard Windows
file extension for your test linkers.
--paulr

>
> Modified:
> cfe/trunk/test/Driver/ps4-linker-win.c
>
> Modified: cfe/trunk/test/Driver/ps4-linker-win.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/ps4-linker-
> win.c?rev=274084=274083=274084=diff
> ==
> 
> --- 

RE: r274246 - [codeview] Emit qualified display names if -gline-tables-only is on

[Robinson, Paul via cfe-commits]

The rationale is no different for DWARF than for CodeView, there's no other way 
to get the fully qualified name of an inlined function.  Unless you want to 
connect this up to the existing DWARF hook for putting linkage names on inlined 
subprograms?  The downside there is that linkage names are probably going to be 
longer than the qualified names, so maybe that's not such a good idea.  Not 
knowing how these names get used by a consumer of –gmlt data, hard for me to 
judge.

IIRC when I put linkage-names in for inlined functions, it added ~1% to the 
normal ("limited") debug-info size.  You'd be adding smaller names to much 
smaller debug-info so the percentage is bound to be bigger.  You'll want to 
collect some hard data but I'd guess it would not be horrifying.
--paulr

From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of Reid 
Kleckner via cfe-commits
Sent: Thursday, June 30, 2016 11:06 AM
To: cfe-commits; David Blaikie; Robinson, Paul
Subject: Re: r274246 - [codeview] Emit qualified display names if 
-gline-tables-only is on

David and Paul,

Do you think I should enable this behavior for DWARF as well? We only emit a 
DW_TAG_subprogram when a function is inlined or contains inlined functions, 
which might not be that many. IIRC Paul did something similar with 
DW_AT_linkage_name, and the object file size cost wasn't that bad.

Reid

On Thu, Jun 30, 2016 at 10:41 AM, Reid Kleckner via cfe-commits 
> wrote:
Author: rnk
Date: Thu Jun 30 12:41:31 2016
New Revision: 274246

URL: http://llvm.org/viewvc/llvm-project?rev=274246=rev
Log:
[codeview] Emit qualified display names if -gline-tables-only is on

When -gmlt is on, we don't emit namespace or class scope information,
and the CodeView emission code in LLVM can't compute the fully qualified
name. If we know LLVM won't be able to get the name right, go ahead and
emit the qualified name in the frontend.

We could change our -gmlt emission strategy to include those scopes when
emitting codeview, but that would increase memory usage and slow down
LTO and add more complexity to debug info emission.

The same problem exists when you debug a -gmlt binary with GDB, so we
should consider removing '&& EmitCodeView' from the condition here at
some point in the future after evaluating the impact on object file
size.

Modified:
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/test/CodeGenCXX/debug-info-codeview-display-name.cpp

Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=274246=274245=274246=diff
==
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Thu Jun 30 12:41:31 2016
@@ -185,17 +185,27 @@ StringRef CGDebugInfo::getFunctionName(c
   FunctionTemplateSpecializationInfo *Info =
   FD->getTemplateSpecializationInfo();

-  if (!Info && FII)
+  // Emit the unqualified name in normal operation. LLVM and the debugger can
+  // compute the fully qualified name from the scope chain. If we're only
+  // emitting line table info, there won't be any scope chains, so emit the
+  // fully qualified name here so that stack traces are more accurate.
+  // FIXME: Do this when emitting DWARF as well as when emitting CodeView after
+  // evaluating the size impact.
+  bool UseQualifiedName = DebugKind == codegenoptions::DebugLineTablesOnly &&
+  CGM.getCodeGenOpts().EmitCodeView;
+
+  if (!Info && FII && !UseQualifiedName)
 return FII->getName();

-  // Otherwise construct human readable name for debug info.
   SmallString<128> NS;
   llvm::raw_svector_ostream OS(NS);
   PrintingPolicy Policy(CGM.getLangOpts());
   Policy.MSVCFormatting = CGM.getCodeGenOpts().EmitCodeView;
+  if (!UseQualifiedName)
+FD->printName(OS);
+  else
+FD->printQualifiedName(OS, Policy);

-  // Print the unqualified name with some template arguments.
-  FD->printName(OS);
   // Add any template specialization args.
   if (Info) {
 const TemplateArgumentList *TArgs = Info->TemplateArguments;

Modified: cfe/trunk/test/CodeGenCXX/debug-info-codeview-display-name.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/debug-info-codeview-display-name.cpp?rev=274246=274245=274246=diff
==
--- cfe/trunk/test/CodeGenCXX/debug-info-codeview-display-name.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/debug-info-codeview-display-name.cpp Thu Jun 30 
12:41:31 2016
@@ -1,14 +1,22 @@
-// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s 
-o - -triple=x86_64-pc-win32 -std=c++98 | \
-// RUN:  grep 'DISubprogram' | sed -e 's/.*name: "\([^"]*\)".*/"\1"/' | 
FileCheck %s
+// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \
+// RUN:   -o - 

RE: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables

[Robinson, Paul via cfe-commits]

About to be away for a week, but I will take this up (and the other one) when I 
get back.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Tuesday, May 17, 2016 3:05 PM
To: reviews+d19567+public+1ee0c82c0824b...@reviews.llvm.org; David Blaikie
Cc: Robinson, Paul; Aaron Ballman; Adrian Prantl; cfe-commits
Subject: Re: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static 
variables

ping

On Mon, May 2, 2016 at 11:51 AM, David Blaikie via cfe-commits 
> wrote:
dblaikie added a comment.

In http://reviews.llvm.org/D19567#414906, @probinson wrote:

> Huh.  There are strange interactions here, which makes me even more nervous 
> about testing fewer cases.


Generally this sort of thing makes me more interested in testing fewer cases so 
we can see/make sure they're properly covering, as you just did by the sounds 
of it. It's hard to see if everything's really covered if there's lots of 
redundant coverage that adds noise to the test case.

> As it happens, the test as written did not exercise all 3 modified paths. 
> Because 'struct S2' had all its members marked with nodebug, none of the 
> static-member references caused debug info for the class as a whole to be 
> attempted.


Not sure I quite follow. Even without nodebug:

  struct foo { static const int x = 3; int y; };
  int i = foo::x;

doesn't produce any debug info for 'foo', x, etc. Just for 'i'.

>   I needed to add a variable of type S2 (without 'nodebug') in order to 
> exercise that path.  Once that was done, then the modification to 
> CollectRecordFields became necessary.

> Even in an unmodified compiler, "static_const_member" never 
> shows up as a DIGlobalVariable, although the other cases all do.  So, testing 
> only for DIGlobalVariable wouldn't be sufficient to show that it gets 
> suppressed by 'nodebug'.


Have you tested cases where the static member is ODR used and/or defined:

  struct foo { static const int x = 3; };
  const int foo::x; // defined
  void sink(void*);
  void use() { sink(::x); } // ODR used

I'm guessing that the out of line definition will create the DIGlobalVariable 
you're not seeing. But probably through a common codepath you've already 
corrected for.

The ODR use probably doesn't cause anything interesting to happen.

>   "static_member" shows up unless we have modified both 
> EmitGlobalVariable(VarDecl case) and CollectRecordFields, given that the test 
> actually tries to emit debug info for S2 as a whole.


I would imagine this could still boil down to: check-not DIGlobalVariable, 
check-not DIFlagStaticMember ?

But once the test is smaller/more targeted, checking the extra details of the 
member list of a composite type, etc, seems OK.

> So, the genuinely most-minimal did-this-change-do-anything test would need 
> only "static_member" and "const_global_int_def" to show that each path had 
> some effect.  This approach does not fill me with warm fuzzies, or the 
> feeling that future changes in this area will not break the intended 
> behavior.  What do you think?

> --paulr


Repository:
  rL LLVM

http://reviews.llvm.org/D19567



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

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

RE: [PATCH] D19754: Allow 'nodebug' on local variables

[Robinson, Paul via cfe-commits]

What you are describing is what testing literature refers to as criteria
for equivalence classes.  There is some level of judgment to that, yes.

Yep yep, to be sure. I'm just generally trying to encourage the community 
behavior towards being both selective & thorough about testing.

I have noticed you doing this (not just in this review) and I am very 
appreciative of the principles; when it comes to understanding what a test is 
trying to do, keeping the unnecessary fluff out is very helpful.  You have no 
idea how many times I've had to suss out the intent of a (usually comment-free) 
test after it broke when we merged it into our tree.  Fortunately that sort of 
thing has been happening less often, now that more of our changes have been 
integrated upstream, but still, it's great to have tests that are very focused….

….when they are tests for a bugfix or other comparatively small change.  I have 
to say when it comes to a new-feature kind of patch, I would rather have the 
test err on the side of completeness.  This is partly based on the experience 
of introducing the 'optnone' attribute to Clang, which IIRC popped up with new 
and surprising cases two or three times after its introduction.  More thorough 
tests up front could easily have prevented those surprises.  Now here I am 
again, not with a new attribute but seriously expanding the applicability of an 
attribute, and would like to apply previous experience and start out with what 
I think should be a moderately complete test.

If you're unwilling to accept that argument and insist on minimal upstream 
tests, okay; I can take what I've done and migrate it into our private tests, 
and leave behind only the minimal upstream test.  It will leave me with the 
test I think the feature needs, leaves upstream with the minimal test you 
prefer, and if something breaks it will just take a little longer to get that 
feedback.
Let me know.
Thanks,
--paulr

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

RE: [PATCH] D19754: Allow 'nodebug' on local variables

[Robinson, Paul via cfe-commits]

This would be a great conversation to have at the social, sadly I will
have to miss it this month.

>> dblaikie wrote:
>>> Doesn't look like the const case is any different from the non-const
>>> case, is it?
>> Given a white-box analysis of the compiler internals, you are correct;
>> this is in contrast to the static const/non-const handling, which *does*
>> use different paths.
>> I am unwilling to trust that the const/non-const paths for locals will
>> forever use the same path.  You could also look at it as "the test is
>> the spec."
>
> But even then - what about any other property of a variable? What if it's
> in a nested scope instead of a top level scope? What if it's declared in
> a condition (if (int x = ...)). What if it's volatile? We could pick
> arbitrary properties that /could/ have an effect on debug info but we
> generally believe to be orthogonal to the feature at hand

What you are describing is what testing literature refers to as criteria
for equivalence classes.  There is some level of judgment to that, yes.

> & I'd say 'const' is in the same group of things.

I would have thought exactly that for the static-storage case, but it is
demonstrably not true.  Therefore, const/not is a valid distinction for
the equivalence classes in the static case.  Needing a separate const
test for the static case, it seems completely appropriate to have the 
same for the auto case.  In other words, in my judgment the storage-class
doesn't seem relevant to the equivalence class criteria for the test.

> (a const local variable still needs storage, etc, - the address can be
> escaped and accessed from elsewhere and determined to be unique

All of those objections apply equally to the static case, and yet the
static case must have separate tests.

> - and both variables in this example could be optimized away entirely
> by the compiler because they're unused, so the const is no worse off
> there in that theoretical concern)

Again not different for the (file-)static case.  If a file-static
variable is not used in the CU there's no reason for it to be emitted.
As it happens I *did* need uses to get the static cases to work, and
(currently) don't need uses to get the local cases to work, so in the
interest of not including elements in the test case that are irrelevant
to the feature-under-test, I didn't add uses of the locals.

This is an argument for doing the test exactly as I did: first run it
to prove debug info IS emitted in the absence of the attribute, then
again to prove debug info is suppressed in the presence of the attribute.
That way if optimization or lack-of-use means the variable is not emitted,
the test can be adjusted to make sure that condition does not apply,
proving that the lack of debug info is properly because of the attribute
and not because of some other irrelevant circumstance.

--paulr

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

RE: [PATCH] D19689: Add Subjects to NoDebugAttr [NFC]

[Robinson, Paul via cfe-commits]

Generally tests test something other than "this program doesn't crash" - should 
it test that we apply the attribute correctly? (either via ast dump, or 
checking the resulting DWARF doesn't have debug info on the relevant entity)

Or is this not actually a new/separate codepath? In which case do we really 
need the test?

It's a –verify test which is about diagnostics, rather than not-crashing.  It 
parallels line 3 of Sema/attr-nodebug.c, which verifies the attribute can be 
applied to a function.  Without this test, you could remove "ObjCMethod" from 
the Subjects line and no test would fail.  I put "ObjCMethod" on the Subjects 
line because the hand-coded condition used "isFunctionOrMethod" which permits 
Objective-C methods.  The new test passes with old and new compilers, 
demonstrating the NFC claim.

Now, if we decide the 'nodebug' attribute should not apply to Objective-C, 
that's fine with me, in which case the new test should verify that a diagnostic 
*is* produced.

I am admittedly clueless about Objective-C, are they handled differently from 
other functions by the time we get to CGDebugInfo?  If there's another path I 
should tweak, I'd like to know about it.
Thanks,
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Thursday, April 28, 2016 4:26 PM
To: reviews+d19689+public+514682b5314c5...@reviews.llvm.org; Robinson, Paul
Cc: Aaron Ballman; cfe-commits
Subject: Re: [PATCH] D19689: Add Subjects to NoDebugAttr [NFC]

LGTM

On Thu, Apr 28, 2016 at 2:10 PM, Paul Robinson via cfe-commits 
> wrote:
probinson created this revision.
probinson added a reviewer: aaron.ballman.
probinson added a subscriber: cfe-commits.

The 'nodebug' attribute had hand-coded constraints; replace those with a 
Subjects line in Attr.td.
Also add a missing test to verify the attribute is okay on an Objective-C 
method.

http://reviews.llvm.org/D19689

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-nodebug.c
  test/SemaObjC/attr-nodebug.m

Index: test/SemaObjC/attr-nodebug.m
===
--- test/SemaObjC/attr-nodebug.m
+++ test/SemaObjC/attr-nodebug.m
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+@interface NSObject
+- (void)doSomething __attribute__((nodebug));
+@end

Generally tests test something other than "this program doesn't crash" - should 
it test that we apply the attribute correctly? (either via ast dump, or 
checking the resulting DWARF doesn't have debug info on the relevant entity)

Or is this not actually a new/separate codepath? In which case do we really 
need the test?

Index: test/Sema/attr-nodebug.c
===
--- test/Sema/attr-nodebug.c
+++ test/Sema/attr-nodebug.c
@@ -3,7 +3,7 @@
 int a __attribute__((nodebug));

 void b() {
-  int b __attribute__((nodebug)); // expected-warning {{'nodebug' only applies 
to variables with static storage duration and functions}}
+  int b __attribute__((nodebug)); // expected-warning {{'nodebug' attribute 
only applies to functions and global variables}}
 }

 void t1() __attribute__((nodebug));
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3572,18 +3572,6 @@
 }

 static void handleNoDebugAttr(Sema , Decl *D, const AttributeList ) {
-  if (const VarDecl *VD = dyn_cast(D)) {
-if (!VD->hasGlobalStorage())
-  S.Diag(Attr.getLoc(),
- diag::warn_attribute_requires_functions_or_static_globals)
-<< Attr.getName();
-  } else if (!isFunctionOrMethod(D)) {
-S.Diag(Attr.getLoc(),
-   diag::warn_attribute_requires_functions_or_static_globals)
-  << Attr.getName();
-return;
-  }
-
   D->addAttr(::new (S.Context)
  NoDebugAttr(Attr.getRange(), S.Context,
  Attr.getAttributeSpellingListIndex()));
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2504,9 +2504,6 @@
 def warn_incomplete_encoded_type : Warning<
   "encoding of %0 type is incomplete because %1 component has unknown 
encoding">,
   InGroup>;
-def warn_attribute_requires_functions_or_static_globals : Warning<
-  "%0 only applies to variables with static storage duration and functions">,
-  InGroup;
 def warn_gnu_inline_attribute_requires_inline : Warning<
   "'gnu_inline' attribute requires function to be marked 'inline',"
   " attribute ignored">,
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -973,6 +973,8 

RE: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables

[Robinson, Paul via cfe-commits]

Huh.  There are strange interactions here, which makes me even more nervous 
about testing fewer cases.
As it happens, the test as written did not exercise all 3 modified paths. 
Because 'struct S2' had all its members marked with nodebug, none of the 
static-member references caused debug info for the class as a whole to be 
attempted.  I needed to add a variable of type S2 (without 'nodebug') in order 
to exercise that path.  Once that was done, then the modification to 
CollectRecordFields became necessary.
   Even in an unmodified compiler, "static_const_member" never 
shows up as a DIGlobalVariable, although the other cases all do.  So, testing 
only for DIGlobalVariable wouldn't be sufficient to show that it gets 
suppressed by 'nodebug'.
   "static_member" shows up unless we have modified both 
EmitGlobalVariable(VarDecl case) and CollectRecordFields, given that the test 
actually tries to emit debug info for S2 as a whole.

So, the genuinely most-minimal did-this-change-do-anything test would need only 
"static_member" and "const_global_int_def" to show that each path had some 
effect.  This approach does not fill me with warm fuzzies, or the feeling that 
future changes in this area will not break the intended behavior.  What do you 
think?
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Wednesday, April 27, 2016 3:43 PM
To: reviews+d19567+public+1ee0c82c0824b...@reviews.llvm.org; Robinson, Paul
Cc: Aaron Ballman; Adrian Prantl; cfe-commits
Subject: Re: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static 
variables



On Wed, Apr 27, 2016 at 3:24 PM, Paul Robinson via cfe-commits 
> wrote:
probinson added a comment.

In http://reviews.llvm.org/D19567#413997, @dblaikie wrote:

> For 3 code paths (that seem fairly independent from one another) I'd only 
> really expect to see 3 variables in the test - one to exercise each codepath. 
> What's the reason for the larger set of test cases?


I'm not interested in testing code-paths, I'm interested in testing a feature, 
and relying on the fact that (at the moment) only 3 code paths are involved is 
not really robust.  Granted there is some duplication, and I took that out 
(r267804), but there are more than 3 relevant cases from an end-user-feature 
perspective.

We generally have to assume /something/ about the implementation to constrain 
testing. eg: we don't assume that putting a variable inside a namespace would 
make a difference here so we don't go & test all these case inside a namespace 
as well as in the global namespace. So I'm not sure why we wouldn't go further 
down that path, knowing that there are only a small number of ways global 
variable definitions can impact debug info generation.


> Then it might be simpler just to include 6 variables, one of each kind with 
> the attribute, one of each kind without. & I think you could check this 
> reliably with:

>

> CHECK: DIGlobalVariable

>  CHECK-NEXT: "name"

>

> repeated three times with a CHECK-NOT: DIGlobalVariable at the end - that way 
> the CHECK wouldn't skip past an existing variable, and you'd check you got 
> the right ones. I think.


If I cared only about DIGlobalVariable that does sound like it would work.  
Sadly, the static const data member comes out as a DIDerivedType, not a 
DIGlobalVariable.  Also, it's *really* important to us that the DICompositeType 
for "S1" is not present; that's actually where we get the major benefit.

But that codepath is already tested by any test for a TU that has a static 
member without a definition, right?

As for the composite type - this is much like testing in LLVM where we test 
that a certain inlining occurred when we test the inliner - not the knock-on 
effect that that has on some other optimization which we've already tested 
separately.

Once you have more than one kind of thing to look for (or -NOT look for) it 
gets a lot more tedious to show it's all correct in one pass like you are 
suggesting.

(Re. getting rid of the types:  We have licensees with major template 
metaprogramming going on, but if they can mark those variables 'nodebug' and 
consequently suppress the instantiated types from the debug info, they should 
see a major size win.  One licensee reported that reducing some name from 19 
characters to 1 char got them a 5MB reduction. Imagine if they could get rid of 
all of those types...)


Repository:
  rL LLVM

http://reviews.llvm.org/D19567


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

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

RE: r266005 - Allow simultaneous safestack and stackprotector attributes.

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Evgeniy Stepanov via cfe-commits
> Sent: Monday, April 11, 2016 3:28 PM
> To: cfe-commits@lists.llvm.org
> Subject: r266005 - Allow simultaneous safestack and stackprotector
> attributes.
> 
> Author: eugenis
> Date: Mon Apr 11 17:27:55 2016
> New Revision: 266005
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=266005=rev
> Log:
> Allow simultaneous safestack and stackprotector attributes.
> 
> This is the clang part of http://reviews.llvm.org/D18846.
> SafeStack instrumentation pass adds stack protector canaries if both
> attributes are present on a function. StackProtector pass will step
> back if the function has a safestack attribute.
> 
> Modified:
> cfe/trunk/lib/Driver/Tools.cpp
> cfe/trunk/test/CodeGen/stack-protector.c
> cfe/trunk/test/Driver/fsanitize.c
> 
> Modified: cfe/trunk/lib/Driver/Tools.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/Driver/Tools.cpp?rev=266005=266004=266005
> =diff
> ==
> 
> --- cfe/trunk/lib/Driver/Tools.cpp (original)
> +++ cfe/trunk/lib/Driver/Tools.cpp Mon Apr 11 17:27:55 2016
> @@ -4878,15 +4878,10 @@ void Clang::ConstructJob(Compilation ,
> 
>// -stack-protector=0 is default.
>unsigned StackProtectorLevel = 0;
> -  if (getToolChain().getSanitizerArgs().needsSafeStackRt()) {
> -Args.ClaimAllArgs(options::OPT_fno_stack_protector);
> -Args.ClaimAllArgs(options::OPT_fstack_protector_all);
> -Args.ClaimAllArgs(options::OPT_fstack_protector_strong);
> -Args.ClaimAllArgs(options::OPT_fstack_protector);
> -  } else if (Arg *A = Args.getLastArg(options::OPT_fno_stack_protector,
> -  options::OPT_fstack_protector_all,
> -
> options::OPT_fstack_protector_strong,
> -  options::OPT_fstack_protector)) {
> +  if (Arg *A = Args.getLastArg(options::OPT_fno_stack_protector,
> +   options::OPT_fstack_protector_all,
> +   options::OPT_fstack_protector_strong,
> +   options::OPT_fstack_protector)) {
>  if (A->getOption().matches(options::OPT_fstack_protector)) {
>StackProtectorLevel = std::max(
>LangOptions::SSPOn,
> 
> Modified: cfe/trunk/test/CodeGen/stack-protector.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/stack-
> protector.c?rev=266005=266004=266005=diff
> ==
> 
> --- cfe/trunk/test/CodeGen/stack-protector.c (original)
> +++ cfe/trunk/test/CodeGen/stack-protector.c Mon Apr 11 17:27:55 2016
> @@ -1,13 +1,13 @@
> -// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 0 | FileCheck -
> check-prefix=NOSSP %s
> -// NOSSP: define {{.*}}void @test1(i8* %msg) #0 {
> -// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 1 | FileCheck -
> check-prefix=WITHSSP %s
> -// WITHSSP: define {{.*}}void @test1(i8* %msg) #0 {
> -// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 2 | FileCheck -
> check-prefix=SSPSTRONG %s
> -// SSPSTRONG: define {{.*}}void @test1(i8* %msg) #0 {
> -// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 3 | FileCheck -
> check-prefix=SSPREQ %s
> -// SSPREQ: define {{.*}}void @test1(i8* %msg) #0 {
> -// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack | FileCheck -
> check-prefix=SAFESTACK %s
> -// SAFESTACK: define {{.*}}void @test1(i8* %msg) #0 {
> +// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 0 | FileCheck -
> check-prefix=DEF -check-prefix=NOSSP %s
> +// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 1 | FileCheck -
> check-prefix=DEF -check-prefix=SSP %s
> +// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 2 | FileCheck -
> check-prefix=DEF -check-prefix=SSPSTRONG %s
> +// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 3 | FileCheck -
> check-prefix=DEF -check-prefix=SSPREQ %s
> +
> +// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack | FileCheck -
> check-prefix=DEF -check-prefix=SAFESTACK-NOSSP %s
> +// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-
> protector 0 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-NOSSP %s
> +// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-
> protector 1 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-SSP %s
> +// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-
> protector 2 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-
> SSPSTRONG %s
> +// RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-
> protector 3 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-SSPREQ
> %s
> 
>  typedef __SIZE_TYPE__ size_t;
> 
> @@ -15,18 +15,21 @@ int printf(const char * _Format, ...);
>  size_t strlen(const char *s);
>  char *strcpy(char *s1, const char *s2);
> 
> +// DEF: define {{.*}}void @test1(i8* 

RE: r265218 - [test] Don't use "UNSUPPORTED" in FileCheck prefixes

[Robinson, Paul via cfe-commits]

Is it worth teaching FileCheck about lit-defined keywords, and
reject check-prefixes that end in one of them?
Offhand that would be UNSUPPORTED, RUN, REQUIRES, XFAIL
(I haven't actually gone to look at what lit knows).
--paulr

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Greg Parker via cfe-commits
> Sent: Friday, April 01, 2016 10:29 PM
> To: cfe-commits@lists.llvm.org
> Subject: r265218 - [test] Don't use "UNSUPPORTED" in FileCheck prefixes
> 
> Author: gparker
> Date: Sat Apr  2 00:29:00 2016
> New Revision: 265218
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=265218=rev
> Log:
> [test] Don't use "UNSUPPORTED" in FileCheck prefixes
> 
> lit uses "UNSUPPORTED:" for its own purposes and may be
> confused if that text appears elsewhere in the test file.
> 
> Modified:
> cfe/trunk/test/Driver/arc.c
> cfe/trunk/test/Driver/objc-weak.m
> 
> Modified: cfe/trunk/test/Driver/arc.c
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/Driver/arc.c?rev=265218=265217=265218=di
> ff
> ==
> 
> --- cfe/trunk/test/Driver/arc.c (original)
> +++ cfe/trunk/test/Driver/arc.c Sat Apr  2 00:29:00 2016
> @@ -3,7 +3,7 @@
>  // RUN: not %clang -x objective-c++ -target i386-apple-darwin10 -m32 -
> fobjc-arc %s -fsyntax-only 2>&1 | FileCheck %s
>  // RUN: not %clang -x c -target i386-apple-darwin10 -m32 -fobjc-arc %s -
> fsyntax-only 2>&1 | FileCheck -check-prefix NOTOBJC %s
>  // RUN: not %clang -x c++ -target i386-apple-darwin10 -m32 -fobjc-arc %s
> -fsyntax-only 2>&1 | FileCheck -check-prefix NOTOBJC %s
> -// RUN: not %clang -x objective-c -target x86_64-apple-darwin11 -mmacosx-
> version-min=10.5 -fobjc-arc %s -fsyntax-only 2>&1 | FileCheck -check-
> prefix UNSUPPORTED %s
> +// RUN: not %clang -x objective-c -target x86_64-apple-darwin11 -mmacosx-
> version-min=10.5 -fobjc-arc %s -fsyntax-only 2>&1 | FileCheck -check-
> prefix NOTSUPPORTED %s
> 
>  // Just to test clang is working.
>  # foo
> @@ -14,4 +14,4 @@
>  // NOTOBJC-NOT: error: -fobjc-arc is not supported on platforms using the
> legacy runtime
>  // NOTOBJC: invalid preprocessing directive
> 
> -// UNSUPPORTED: error: -fobjc-arc is not supported on versions of OS X
> prior to 10.6
> +// NOTSUPPORTED: error: -fobjc-arc is not supported on versions of OS X
> prior to 10.6
> 
> Modified: cfe/trunk/test/Driver/objc-weak.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/objc-
> weak.m?rev=265218=265217=265218=diff
> ==
> 
> --- cfe/trunk/test/Driver/objc-weak.m (original)
> +++ cfe/trunk/test/Driver/objc-weak.m Sat Apr  2 00:29:00 2016
> @@ -10,9 +10,9 @@
>  // ARC-NO-WEAK: -fobjc-arc
>  // ARC-NO-WEAK: -fno-objc-weak
> 
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-arc -fobjc-weak 2>&1 | FileCheck %s --check-prefix ARC-WEAK-
> UNSUPPORTED
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fobjc-weak -fobjc-arc  2>&1 | FileCheck %s --check-
> prefix ARC-WEAK-UNSUPPORTED
> -// ARC-WEAK-UNSUPPORTED: error: -fobjc-weak is not supported on the
> current deployment target
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-arc -fobjc-weak 2>&1 | FileCheck %s --check-prefix ARC-WEAK-
> NOTSUPPORTED
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fobjc-weak -fobjc-arc  2>&1 | FileCheck %s --check-
> prefix ARC-WEAK-NOTSUPPORTED
> +// ARC-WEAK-NOTSUPPORTED: error: -fobjc-weak is not supported on the
> current deployment target
> 
>  // RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.7 -S -
> ### %s -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-WEAK
>  // RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.7 -S -
> ### %s -fno-objc-weak -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-
> WEAK
> @@ -22,6 +22,6 @@
>  // RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.7 -S -
> ### %s -fobjc-weak -fno-objc-weak 2>&1 | FileCheck %s --check-prefix MRC-
> NO-WEAK
>  // MRC-NO-WEAK: -fno-objc-weak
> 
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-WEAK-UNSUPPORTED
> -// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-
> WEAK-UNSUPPORTED
> -// MRC-WEAK-UNSUPPORTED: error: -fobjc-weak is not supported on the
> current deployment target
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fobjc-weak 2>&1 | FileCheck %s --check-prefix MRC-WEAK-
> NOTSUPPORTED
> +// RUN: %clang -target x86_64-apple-macosx -mmacosx-version-min=10.5 -S -
> ### %s -fno-objc-weak -fobjc-weak 2>&1 | FileCheck 

RE: r262688 - [X86] Pass __m64 types via SSE registers for GCC compatibility

[Robinson, Paul via cfe-commits]

> Ah, great! I always love it when people document their ABIs.
> 
> Is your ABI document public? If so, could you add it to
> docs/CompilerWriterInfo.rst?

I don't think I was aware of that page...  AFAIK ours is not available
on-line, but I can raise the question internally.
--paulr

> 
> On Fri, Mar 4, 2016 at 11:54 AM, Robinson, Paul
> <paul_robin...@playstation.sony.com> wrote:
> >> It'd be nice to have a comment here that mentions that the clang
> >> behavior which is being preserved for Darwin, FreeBSD, and PS4 is a
> >> *bug* which is being intentionally left unfixed. The previous clang
> >> behavior directly contradicts the x86_64 ABI document, which I believe
> >> all of these platforms claim to follow. :)
> >
> > Well, PS4 uses x86_64 ABI as a base document, but we have a handful of
> > variances.  We had already documented this one to our licensees.  So,
> > from our perspective, it's not a bug, it's a feature. :-)  Describing
> > it as a bug (at least for PS4) would be technically incorrect.
> > --paulr
> >
> >>
> >> On Fri, Mar 4, 2016 at 2:03 AM, Robinson, Paul via cfe-commits
> >> <cfe-commits@lists.llvm.org> wrote:
> >> >> To: cfe-commits@lists.llvm.org
> >> >> Subject: r262688 - [X86] Pass __m64 types via SSE registers for GCC
> >> >> compatibility
> >> >>
> >> >> Author: majnemer
> >> >> Date: Thu Mar  3 23:26:16 2016
> >> >> New Revision: 262688
> >> >>
> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=262688=rev
> >> >> Log:
> >> >> [X86] Pass __m64 types via SSE registers for GCC compatibility
> >> >>
> >> >> For compatibility with GCC, classify __m64 as SSE.
> >> >> However, clang is a platform compiler for certain targets; retain
> our
> >> >> old behavior on those targets: classify __m64 as integer.
> >> >
> >> > Thank you very much for that!
> >> > --paulr
> >> >
> >> >>
> >> >> This fixes PR26832.
> >> >>
> >> >> Modified:
> >> >> cfe/trunk/lib/CodeGen/TargetInfo.cpp
> >> >> cfe/trunk/test/CodeGen/3dnow-builtins.c
> >> >> cfe/trunk/test/CodeGen/x86_64-arguments.c
> >> >>
> >> >> Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
> >> >> URL: http://llvm.org/viewvc/llvm-
> >> >>
> >>
> project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=262688=262687=26268
> >> >> 8=diff
> >> >>
> >>
> ==
> >> >> 
> >> >> --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
> >> >> +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Thu Mar  3 23:26:16 2016
> >> >> @@ -1857,6 +1857,17 @@ class X86_64ABIInfo : public ABIInfo {
> >> >>  return !getTarget().getTriple().isOSDarwin();
> >> >>}
> >> >>
> >> >> +  /// GCC classifies <1 x long long> as SSE but compatibility with
> >> older
> >> >> clang
> >> >> +  // compilers require us to classify it as INTEGER.
> >> >> +  bool classifyIntegerMMXAsSSE() const {
> >> >> +const llvm::Triple  = getTarget().getTriple();
> >> >> +if (Triple.isOSDarwin() || Triple.getOS() == llvm::Triple::PS4)
> >> >> +  return false;
> >> >> +if (Triple.isOSFreeBSD() && Triple.getOSMajorVersion() >= 10)
> >> >> +  return false;
> >> >> +return true;
> >> >> +  }
> >> >> +
> >> >>X86AVXABILevel AVXLevel;
> >> >>// Some ABIs (e.g. X32 ABI and Native Client OS) use 32 bit
> pointers
> >> on
> >> >>// 64-bit hardware.
> >> >> @@ -2298,15 +2309,20 @@ void X86_64ABIInfo::classify(QualType Ty
> >> >>if (EB_Lo != EB_Hi)
> >> >>  Hi = Lo;
> >> >>  } else if (Size == 64) {
> >> >> +  QualType ElementType = VT->getElementType();
> >> >> +
> >> >>// gcc passes <1 x double> in memory. :(
> >> >> -  if (VT->getElementType()-
> >> >> >isSpecificBuiltinType(BuiltinType::Double))
> >> >> +  if (ElementType->isSpecificBuiltinType(BuiltinType::Double))
> >> >>

RE: r262688 - [X86] Pass __m64 types via SSE registers for GCC compatibility

[Robinson, Paul via cfe-commits]

> It'd be nice to have a comment here that mentions that the clang
> behavior which is being preserved for Darwin, FreeBSD, and PS4 is a
> *bug* which is being intentionally left unfixed. The previous clang
> behavior directly contradicts the x86_64 ABI document, which I believe
> all of these platforms claim to follow. :)

Well, PS4 uses x86_64 ABI as a base document, but we have a handful of
variances.  We had already documented this one to our licensees.  So,
from our perspective, it's not a bug, it's a feature. :-)  Describing
it as a bug (at least for PS4) would be technically incorrect.
--paulr

> 
> On Fri, Mar 4, 2016 at 2:03 AM, Robinson, Paul via cfe-commits
> <cfe-commits@lists.llvm.org> wrote:
> >> To: cfe-commits@lists.llvm.org
> >> Subject: r262688 - [X86] Pass __m64 types via SSE registers for GCC
> >> compatibility
> >>
> >> Author: majnemer
> >> Date: Thu Mar  3 23:26:16 2016
> >> New Revision: 262688
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=262688=rev
> >> Log:
> >> [X86] Pass __m64 types via SSE registers for GCC compatibility
> >>
> >> For compatibility with GCC, classify __m64 as SSE.
> >> However, clang is a platform compiler for certain targets; retain our
> >> old behavior on those targets: classify __m64 as integer.
> >
> > Thank you very much for that!
> > --paulr
> >
> >>
> >> This fixes PR26832.
> >>
> >> Modified:
> >> cfe/trunk/lib/CodeGen/TargetInfo.cpp
> >> cfe/trunk/test/CodeGen/3dnow-builtins.c
> >> cfe/trunk/test/CodeGen/x86_64-arguments.c
> >>
> >> Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
> >> URL: http://llvm.org/viewvc/llvm-
> >>
> project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=262688=262687=26268
> >> 8=diff
> >>
> ==
> >> 
> >> --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
> >> +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Thu Mar  3 23:26:16 2016
> >> @@ -1857,6 +1857,17 @@ class X86_64ABIInfo : public ABIInfo {
> >>  return !getTarget().getTriple().isOSDarwin();
> >>}
> >>
> >> +  /// GCC classifies <1 x long long> as SSE but compatibility with
> older
> >> clang
> >> +  // compilers require us to classify it as INTEGER.
> >> +  bool classifyIntegerMMXAsSSE() const {
> >> +const llvm::Triple  = getTarget().getTriple();
> >> +if (Triple.isOSDarwin() || Triple.getOS() == llvm::Triple::PS4)
> >> +  return false;
> >> +if (Triple.isOSFreeBSD() && Triple.getOSMajorVersion() >= 10)
> >> +  return false;
> >> +return true;
> >> +  }
> >> +
> >>X86AVXABILevel AVXLevel;
> >>// Some ABIs (e.g. X32 ABI and Native Client OS) use 32 bit pointers
> on
> >>// 64-bit hardware.
> >> @@ -2298,15 +2309,20 @@ void X86_64ABIInfo::classify(QualType Ty
> >>if (EB_Lo != EB_Hi)
> >>  Hi = Lo;
> >>  } else if (Size == 64) {
> >> +  QualType ElementType = VT->getElementType();
> >> +
> >>// gcc passes <1 x double> in memory. :(
> >> -  if (VT->getElementType()-
> >> >isSpecificBuiltinType(BuiltinType::Double))
> >> +  if (ElementType->isSpecificBuiltinType(BuiltinType::Double))
> >>  return;
> >>
> >> -  // gcc passes <1 x long long> as INTEGER.
> >> -  if (VT->getElementType()-
> >> >isSpecificBuiltinType(BuiltinType::LongLong) ||
> >> -  VT->getElementType()-
> >> >isSpecificBuiltinType(BuiltinType::ULongLong) ||
> >> -  VT->getElementType()-
> >isSpecificBuiltinType(BuiltinType::Long)
> >> ||
> >> -  VT->getElementType()-
> >> >isSpecificBuiltinType(BuiltinType::ULong))
> >> +  // gcc passes <1 x long long> as SSE but clang used to
> >> unconditionally
> >> +  // pass them as integer.  For platforms where clang is the de
> facto
> >> +  // platform compiler, we must continue to use integer.
> >> +  if (!classifyIntegerMMXAsSSE() &&
> >> +  (ElementType->isSpecificBuiltinType(BuiltinType::LongLong)
> ||
> >> +   ElementType->isSpecificBuiltinType(BuiltinType::ULongLong)
> ||
> >> +   ElementType->isSpecificBuiltinType(BuiltinType::Long) ||
> >> +   

RE: r250293 - Bring back r250262: PS4 toolchain

[Robinson, Paul via cfe-commits]

Nico asks:

+def warn_drv_ps4_force_pic : Warning<
+  "option '%0' was ignored by the PS4 toolchain, using '-fPIC'">,
+  InGroup;

Should this use the existing UnusedCommandLineArgument instead of introducing a 
new group?

Depends on what kinds of distinctions you want to make, when it comes to 
how-to-turn-off-warnings.  OptionIgnored, IgnoredOptimizationArgument, 
InvalidCommandLineArgument, and UnusedCommandLineArgument all seem to be 
somewhat related; each of the first three has only one warning in the group, 
maybe they could all be folded into the last one.
Looks like there are other similar warnings that currently have no group but 
probably could be folded into the group, e.g. warn_c_kext.

Is there guidance/documentation about how groups ought to be used and when it's 
appropriate to define a new one?  Or is it more like "check around for an 
existing group that seems to fit"?
Thanks,
--paulr

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

RE: r260334 - Get rid of CHECK-SAME-NOT in tests.

[Robinson, Paul via cfe-commits]

Well I'll be-- thanks!
See post-commit comments, see below, tidying up just a bit.
--paulr

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Justin Lebar via cfe-commits
> Sent: Tuesday, February 09, 2016 4:38 PM
> To: cfe-commits@lists.llvm.org
> Subject: r260334 - Get rid of CHECK-SAME-NOT in tests.
> 
> Author: jlebar
> Date: Tue Feb  9 18:38:15 2016
> New Revision: 260334
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=260334=rev
> Log:
> Get rid of CHECK-SAME-NOT in tests.
> 
> Summary: This isn't a FileCheck directive; it does nothing.
> 
> Reviewers: jroelofs
> 
> Subscribers: cfe-commits, majnemer
> 
> Differential Revision: http://reviews.llvm.org/D17051
> 
> Modified:
> cfe/trunk/test/CodeGenCXX/optnone-and-attributes.cpp
> cfe/trunk/test/CodeGenCXX/optnone-class-members.cpp
> cfe/trunk/test/CodeGenCXX/optnone-def-decl.cpp
> cfe/trunk/test/CodeGenCXX/optnone-templates.cpp
> cfe/trunk/test/Modules/ModuleDebugInfo.cpp
> cfe/trunk/test/Modules/ModuleDebugInfo.m
> 
> Modified: cfe/trunk/test/CodeGenCXX/optnone-and-attributes.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/CodeGenCXX/optnone-and-
> attributes.cpp?rev=260334=260333=260334=diff
> ==
> 
> --- cfe/trunk/test/CodeGenCXX/optnone-and-attributes.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/optnone-and-attributes.cpp Tue Feb  9
> 18:38:15 2016
> @@ -79,4 +79,4 @@ int exported_optnone_func(int a) {
>  // CHECK: attributes [[NORETURN]] = { noinline noreturn {{.*}} optnone
> 
>  // CHECK: attributes [[DLLIMPORT]] =
> -// CHECK-SAME-NOT: optnone
> +// CHECK-NOT: optnone
> 
> Modified: cfe/trunk/test/CodeGenCXX/optnone-class-members.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/CodeGenCXX/optnone-class-
> members.cpp?rev=260334=260333=260334=diff
> ==
> 
> --- cfe/trunk/test/CodeGenCXX/optnone-class-members.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/optnone-class-members.cpp Tue Feb  9
> 18:38:15 2016
> @@ -159,6 +159,6 @@ int bar() {
> 
> 
>  // CHECK: attributes [[NORMAL]] =
> -// CHECK-SAME-NOT: noinline
> -// CHECK-SAME-NOT: optnone
> +// CHECK-NOT: noinline
> +// CHECK-NOT: optnone
>  // CHECK: attributes [[OPTNONE]] = {{.*}} noinline {{.*}} optnone
> 
> Modified: cfe/trunk/test/CodeGenCXX/optnone-def-decl.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/CodeGenCXX/optnone-def-
> decl.cpp?rev=260334=260333=260334=diff
> ==
> 
> --- cfe/trunk/test/CodeGenCXX/optnone-def-decl.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/optnone-def-decl.cpp Tue Feb  9 18:38:15
> 2016
> @@ -91,5 +91,5 @@ int user_of_forceinline_optnone_function
> 
>  // CHECK: attributes [[OPTNONE]] = { noinline nounwind optnone {{.*}} }
>  // CHECK: attributes [[NORMAL]] =
> -// CHECK-SAME-NOT: noinline
> -// CHECK-SAME-NOT: optnone
> +// CHECK-NOT: noinline
> +// CHECK-NOT: optnone
> 
> Modified: cfe/trunk/test/CodeGenCXX/optnone-templates.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/CodeGenCXX/optnone-
> templates.cpp?rev=260334=260333=260334=diff
> ==
> 
> --- cfe/trunk/test/CodeGenCXX/optnone-templates.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/optnone-templates.cpp Tue Feb  9 18:38:15
> 2016
> @@ -100,5 +100,5 @@ void container3()
> 
> 
>  // CHECK: attributes [[NORMAL]] =
> -// CHECK-SAME-NOT: optnone
> +// CHECK-NOT: optnone
>  // CHECK: attributes [[OPTNONE]] = {{.*}} optnone
> 
> Modified: cfe/trunk/test/Modules/ModuleDebugInfo.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/test/Modules/ModuleDebugInfo.cpp?rev=260334=260333
> =260334=diff
> ==
> 
> --- cfe/trunk/test/Modules/ModuleDebugInfo.cpp (original)
> +++ cfe/trunk/test/Modules/ModuleDebugInfo.cpp Tue Feb  9 18:38:15 2016
> @@ -20,25 +20,29 @@
> 
>  // CHECK: distinct !DICompileUnit(language: DW_LANG_{{.*}}C_plus_plus,
>  // CHECK-SAME:isOptimized: false,
> -// CHECK-SAME-NOT:splitDebugFilename:
> -// CHECK: dwoId:
> +// CHECK-NOT: splitDebugFilename:
> +// CHECK-SAME:dwoId:
> +// CHECK-SAME:)

The paren check looks redundant.

> 
>  // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Enum"
>  // CHECK-SAME: identifier: "_ZTSN8DebugCXX4EnumE")
>  // CHECK: !DINamespace(name: "DebugCXX"
> 
>  // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type,
> -// CHECK-SAME-NOT: name:
> +// CHECK-NOT:  name:
> +// CHECK-SAME: )
> 
>  // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type,
> -// 

RE: r259489 - Move DebugInfoKind into its own header to cut the cyclic dependency edge from Driver to Frontend.

[Robinson, Paul via cfe-commits]

Maybe Basic would be a better home for the new header?
Having CodeGen pull in something from Driver seems weird and unprecedented.
Thanks,
--paulr

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Benjamin Kramer via cfe-commits
> Sent: Tuesday, February 02, 2016 3:07 AM
> To: cfe-commits@lists.llvm.org
> Subject: r259489 - Move DebugInfoKind into its own header to cut the
> cyclic dependency edge from Driver to Frontend.
> 
> Author: d0k
> Date: Tue Feb  2 05:06:51 2016
> New Revision: 259489
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=259489=rev
> Log:
> Move DebugInfoKind into its own header to cut the cyclic dependency edge
> from Driver to Frontend.
> 
> Added:
> cfe/trunk/include/clang/Driver/DebugInfoKind.h
> Modified:
> cfe/trunk/include/clang/Frontend/CodeGenOptions.def
> cfe/trunk/include/clang/Frontend/CodeGenOptions.h
> cfe/trunk/lib/CodeGen/BackendUtil.cpp
> cfe/trunk/lib/CodeGen/CGBlocks.cpp
> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> cfe/trunk/lib/CodeGen/CGDebugInfo.h
> cfe/trunk/lib/CodeGen/CGDecl.cpp
> cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
> cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
> cfe/trunk/lib/CodeGen/CodeGenModule.cpp
> cfe/trunk/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
> cfe/trunk/lib/Driver/Tools.cpp
> cfe/trunk/lib/Driver/Tools.h
> cfe/trunk/lib/Frontend/CompilerInvocation.cpp
> cfe/trunk/lib/Frontend/Rewrite/FrontendActions.cpp
> 
> Added: cfe/trunk/include/clang/Driver/DebugInfoKind.h
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/include/clang/Driver/DebugInfoKind.h?rev=259489=aut
> o
> ==
> 
> --- cfe/trunk/include/clang/Driver/DebugInfoKind.h (added)
> +++ cfe/trunk/include/clang/Driver/DebugInfoKind.h Tue Feb  2 05:06:51
> 2016
> @@ -0,0 +1,39 @@
> +//===--- DebugInfoKind.h - Debug Info Emission Types *- C++ -
> *-===//
> +//
> +// The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===
> --===//
> +
> +#ifndef LLVM_CLANG_DRIVER_DEBUGINFOKIND_H
> +#define LLVM_CLANG_DRIVER_DEBUGINFOKIND_H
> +
> +namespace clang {
> +namespace codegenoptions {
> +
> +enum DebugInfoKind {
> +  NoDebugInfo, /// Don't generate debug info.
> +  LocTrackingOnly, /// Emit location information but do not generate
> +   /// debug info in the output. This is useful in
> +   /// cases where the backend wants to track source
> +   /// locations for instructions without actually
> +   /// emitting debug info for them (e.g., when -
> Rpass
> +   /// is used).
> +  DebugLineTablesOnly, /// Emit only debug info necessary for generating
> +   /// line number tables (-gline-tables-only).
> +  LimitedDebugInfo,/// Limit generated debug info to reduce size
> +   /// (-fno-standalone-debug). This emits
> +   /// forward decls for types that could be
> +   /// replaced with forward decls in the source
> +   /// code. For dynamic C++ classes type info
> +   /// is only emitted int the module that
> +   /// contains the classe's vtable.
> +  FullDebugInfo/// Generate complete debug info.
> +};
> +
> +} // end namespace codegenoptions
> +} // end namespace clang
> +
> +#endif
> 
> Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.def
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/include/clang/Frontend/CodeGenOptions.def?rev=259489=
> 259488=259489=diff
> ==
> 
> --- cfe/trunk/include/clang/Frontend/CodeGenOptions.def (original)
> +++ cfe/trunk/include/clang/Frontend/CodeGenOptions.def Tue Feb  2
> 05:06:51 2016
> @@ -185,7 +185,7 @@ VALUE_CODEGENOPT(NumRegisterParameters,
>  VALUE_CODEGENOPT(SSPBufferSize, 32, 0)
> 
>  /// The kind of generated debug info.
> -ENUM_CODEGENOPT(DebugInfo, DebugInfoKind, 3, NoDebugInfo)
> +ENUM_CODEGENOPT(DebugInfo, codegenoptions::DebugInfoKind, 3,
> codegenoptions::NoDebugInfo)
> 
>  /// Tune the debug info for this debugger.
>  ENUM_CODEGENOPT(DebuggerTuning, DebuggerKind, 2, DebuggerKindDefault)
> 
> Modified: cfe/trunk/include/clang/Frontend/CodeGenOptions.h
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/include/clang/Frontend/CodeGenOptions.h?rev=259489=25
> 9488=259489=diff
> ==
> 
> --- cfe/trunk/include/clang/Frontend/CodeGenOptions.h (original)
> +++ cfe/trunk/include/clang/Frontend/CodeGenOptions.h Tue Feb 

RE: [PATCH] D16754: Bug 15785 - OpenCL errors using vector/scalar conditionals and short integer types

[Robinson, Paul via cfe-commits]

> Hi Anton,
> 
>   Okey, nevermore.
>   But what to do with a bug which needs simply be closed (NABs)?
>   I believe they also need a review/approval.
> 
> Igor

Bugs do not have such a strict process, compared to patches.
If you don't feel confident in closing the bug directly, you can ask
the person who submitted the bug if they are happy with your answer.
--paulr

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

RE: [libcxx] r196411 - Give all members of exception types default visibility. Lack of this is causing some illegal code relocations rare and hard to reproduce cases.

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Duncan P. N. Exon Smith via cfe-commits
> Sent: Monday, January 11, 2016 4:21 PM
> To: Rafael Espíndola
> Cc: Marshall Clow; CFE Commits
> Subject: Re: [libcxx] r196411 - Give all members of exception types
> default visibility. Lack of this is causing some illegal code relocations
> rare and hard to reproduce cases.
> 
> 
> > On 2016-Jan-11, at 15:57, Rafael Espíndola 
> wrote:
> >
> >> I'm not sure about GCC.  Note that there is no linkage for -frtti,
> >> since the type info is deferred to the TU with the vtable.
> >
> > Yes, that is what I mean. It is odd that -frtti changes us from "this
> > is not available anywhere, use linkonce_odr" to "it is available
> > elsewhere, use an external declaration".
> 
> Yes, I agree, it's weird (although the transition is in the other
> direction, really, since there's no such flag as -frtti, just -fno-rtti).

Drive-by comment:
Actually there is -frtti, it's just not defined next to -fno-rtti in
Options.td for some reason.

> -fexceptions -fno-rtti is a weird combination to begin with though.

Yup. So weird that PS4 goes to the trouble of forbidding it... that
doesn't help here, unfortunately.
--paulr

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

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

Actually no, we prefer to have the original typedef names in the instantiation 
name, for source fidelity.  "Name as it is in the source" or something 
reasonably close.  Unwrapping typedefs is going too far.

Re. "looseness" of the DWARF spec, it is not so loose as you like to think.  
Attributes tend to be fairly optional or can be used "in novel ways" but the 
DIEs and their relationships are not like that.  "Where this specification 
provides a means for describing the source language, implementors are expected 
to adhere to that specification."
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Wednesday, December 09, 2015 10:49 AM
To: Robinson, Paul
Cc: Marshall, Peter; llvm-dev; cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Wed, Dec 9, 2015 at 10:40 AM, Robinson, Paul 
> 
wrote:
That doesn't seem to be the DWARF I'm seeing from Clang (& it'd be surprising 
if we used the typedef (or otherwise non-canonical) name in the class name):

Finally getting back to this…..  Ha.  We don't unwrap the typedefs ("name as it 
is in the source"), while the upstream compiler does.

Yeah, I imagine you'd want to fix that as I expect it would cause you other 
problems, no? (or is there some reason you have this change to the compiler? I 
imagine it'd be hard to have that divergence by accident?)

Providing the template-parameter DIEs is still the correct thing to do per the 
DWARF
spec.

I still don't agree that the DWARF we produce here is incorrect (the DWARF spec 
is pretty loose on "correctness" of DWARF). If there's some practical 
problem/use case it'd be useful to understand it so we make sure we're fixing 
it the right way.

- Dave

--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Friday, November 13, 2015 11:21 AM
To: Marshall, Peter; llvm-dev
Cc: Robinson, Paul

Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Fri, Nov 13, 2015 at 6:16 AM, 
> wrote:
Hi Paul,

Sorry for the delay, I've been out of the office.

I think this example shows that name matching does not always work:

template class A {
public:
A(T val);
private:
T x;
};

struct B {
typedef float MONKEY;

A *p;
};

B b;

struct C {
typedef int MONKEY;

A *p;
};

C c;

This gives this DWARF:

+-003f DW_TAG_structure_type "B"
   -DW_AT_name  DW_FORM_strp  "B"
  +-0047 DW_TAG_member "p"
 -DW_AT_name  DW_FORM_strp  "p"
+-DW_AT_type  DW_FORM_ref4  0x0054
  +-0054 DW_TAG_pointer_type
+-DW_AT_type  DW_FORM_ref4  0x0059
  +-0059 DW_TAG_class_type "A"
 -DW_AT_name  DW_FORM_strp  "A"
 -DW_AT_declaration  DW_FORM_flag_present

+-0073 DW_TAG_structure_type "C"
   -DW_AT_name  DW_FORM_strp  "C"
  +-007b DW_TAG_member "p"
 -DW_AT_name  DW_FORM_strp  "p"
+-DW_AT_type  DW_FORM_ref4  0x0088
  +-0088 DW_TAG_pointer_type
+-DW_AT_type  DW_FORM_ref4  0x008d
  +-008d DW_TAG_class_type "A"
 -DW_AT_name  DW_FORM_strp  "A"
 -DW_AT_declaration  DW_FORM_flag_present

That doesn't seem to be the DWARF I'm seeing from Clang (& it'd be surprising 
if we used the typedef (or otherwise non-canonical) name in the class name):

(I've trimmed a few irrelevant attributes)
0x001e:   DW_TAG_variable [2]
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x004c] = "b")
DW_AT_type [DW_FORM_ref4]   (cu + 0x0033 => {0x0033})

0x0033:   DW_TAG_structure_type [3] *
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x0059] = "B")

0x003b: DW_TAG_member [4]
  DW_AT_name [DW_FORM_strp] ( .debug_str[0x004e] = "p")
  DW_AT_type [DW_FORM_ref4] (cu + 0x0048 => {0x0048})

0x0047: NULL

0x0048:   DW_TAG_pointer_type [5]
DW_AT_type [DW_FORM_ref4]   (cu + 0x004d => {0x004d})

0x004d:   DW_TAG_class_type [6]
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x0050] = 
"A")
DW_AT_declaration [DW_FORM_flag_present](true)

0x0052:   DW_TAG_variable [2]
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x005b] = "c")
DW_AT_type [DW_FORM_ref4]   (cu + 0x0067 => {0x0067})

0x0067:   DW_TAG_structure_type [3] *
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x0064] = "C")

0x006f: DW_TAG_member [4]
  DW_AT_name [DW_FORM_strp] ( .debug_str[0x004e] = "p")
  DW_AT_type [DW_FORM_ref4] (cu + 0x007c => {0x007c})

0x007b: NULL

0x007c:   DW_TAG_pointer_type 

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

That doesn't seem to be the DWARF I'm seeing from Clang (& it'd be surprising 
if we used the typedef (or otherwise non-canonical) name in the class name):

Finally getting back to this…..  Ha.  We don't unwrap the typedefs ("name as it 
is in the source"), while the upstream compiler does.
Providing the template-parameter DIEs is still the correct thing to do per the 
DWARF spec.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Friday, November 13, 2015 11:21 AM
To: Marshall, Peter; llvm-dev
Cc: Robinson, Paul
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Fri, Nov 13, 2015 at 6:16 AM, 
> wrote:
Hi Paul,

Sorry for the delay, I've been out of the office.

I think this example shows that name matching does not always work:

template class A {
public:
A(T val);
private:
T x;
};

struct B {
typedef float MONKEY;

A *p;
};

B b;

struct C {
typedef int MONKEY;

A *p;
};

C c;

This gives this DWARF:

+-003f DW_TAG_structure_type "B"
   -DW_AT_name  DW_FORM_strp  "B"
  +-0047 DW_TAG_member "p"
 -DW_AT_name  DW_FORM_strp  "p"
+-DW_AT_type  DW_FORM_ref4  0x0054
  +-0054 DW_TAG_pointer_type
+-DW_AT_type  DW_FORM_ref4  0x0059
  +-0059 DW_TAG_class_type "A"
 -DW_AT_name  DW_FORM_strp  "A"
 -DW_AT_declaration  DW_FORM_flag_present

+-0073 DW_TAG_structure_type "C"
   -DW_AT_name  DW_FORM_strp  "C"
  +-007b DW_TAG_member "p"
 -DW_AT_name  DW_FORM_strp  "p"
+-DW_AT_type  DW_FORM_ref4  0x0088
  +-0088 DW_TAG_pointer_type
+-DW_AT_type  DW_FORM_ref4  0x008d
  +-008d DW_TAG_class_type "A"
 -DW_AT_name  DW_FORM_strp  "A"
 -DW_AT_declaration  DW_FORM_flag_present

That doesn't seem to be the DWARF I'm seeing from Clang (& it'd be surprising 
if we used the typedef (or otherwise non-canonical) name in the class name):

(I've trimmed a few irrelevant attributes)
0x001e:   DW_TAG_variable [2]
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x004c] = "b")
DW_AT_type [DW_FORM_ref4]   (cu + 0x0033 => {0x0033})

0x0033:   DW_TAG_structure_type [3] *
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x0059] = "B")

0x003b: DW_TAG_member [4]
  DW_AT_name [DW_FORM_strp] ( .debug_str[0x004e] = "p")
  DW_AT_type [DW_FORM_ref4] (cu + 0x0048 => {0x0048})

0x0047: NULL

0x0048:   DW_TAG_pointer_type [5]
DW_AT_type [DW_FORM_ref4]   (cu + 0x004d => {0x004d})

0x004d:   DW_TAG_class_type [6]
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x0050] = 
"A")
DW_AT_declaration [DW_FORM_flag_present](true)

0x0052:   DW_TAG_variable [2]
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x005b] = "c")
DW_AT_type [DW_FORM_ref4]   (cu + 0x0067 => {0x0067})

0x0067:   DW_TAG_structure_type [3] *
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x0064] = "C")

0x006f: DW_TAG_member [4]
  DW_AT_name [DW_FORM_strp] ( .debug_str[0x004e] = "p")
  DW_AT_type [DW_FORM_ref4] (cu + 0x007c => {0x007c})

0x007b: NULL

0x007c:   DW_TAG_pointer_type [5]
DW_AT_type [DW_FORM_ref4]   (cu + 0x0081 => {0x0081})

0x0081:   DW_TAG_class_type [6]
DW_AT_name [DW_FORM_strp]   ( .debug_str[0x005d] = 
"A")
DW_AT_declaration [DW_FORM_flag_present](true)



As there are no template parameters for the forward declaration of either 
A
they are indistinguishable.

The reason we currently have no need for the parameters in a template name is 
because we
reconstruct template names from their parameter tags. This allow the pretty 
printing to match
the templates from the DWARF to match our demangled symbols from the ELF symbol 
table.

-Pete




From:"Robinson, Paul" 
>
To:David Blaikie >, 
"Marshall, Peter" 
>
Cc:
"reviews+d14358+public+d3104135076f0...@reviews.llvm.org"

>,
 "cfe-commits (cfe-commits@lists.llvm.org)" 
>
Date:10/11/2015 01:08
Subject:RE: [PATCH] D14358: DWARF's forward decl of a template should 
have template parameters.

RE: r254574 - PR17381: Treat undefined behavior during expression evaluation as an unmodeled

[Robinson, Paul via cfe-commits]

| And at runtime, on some targets, we use this:
|
|  
https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/builtins/floatuntisf.c
|
| ... which gives a NaN in this case.

I copied that function into a test program on Ubuntu, built with gcc, and it 
gives me +Infinity (0x7f80) not NaN (0x7fc0).
--paulr

From: meta...@gmail.com [mailto:meta...@gmail.com] On Behalf Of Richard Smith
Sent: Tuesday, December 08, 2015 11:42 AM
To: Robinson, Paul
Cc: Joerg Sonnenberger; cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

On Tue, Dec 8, 2015 at 11:18 AM, Richard Smith 
> wrote:
On Tue, Dec 8, 2015 at 10:59 AM, Robinson, Paul 
> 
wrote:
Okay, I'll bite:  so what *does* UINT128_MAX actually convert to?

$ echo 'unsigned __int128 max = -1; float f = max;' | ~/clang-8/build/bin/clang 
-x c++ - -emit-llvm -S -o - -O3 | grep @f
@f = global float undef, align 4

And at runtime, on some targets, we use this:

  https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/builtins/floatuntisf.c

... which gives a NaN in this case.

From: cfe-commits 
[mailto:cfe-commits-boun...@lists.llvm.org]
 On Behalf Of Richard Smith via cfe-commits
Sent: Tuesday, December 08, 2015 10:52 AM
To: Joerg Sonnenberger; cfe-commits
Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

On Tue, Dec 8, 2015 at 2:13 AM, Joerg Sonnenberger via cfe-commits 
> wrote:
On Mon, Dec 07, 2015 at 01:32:14PM -0800, Richard Smith via cfe-commits wrote:
> C11 6.3.1.5/1: "If the value being converted is outside the 
> range of values
> that can be represented, the behavior is undefined."

The value of 1e100 can be represented as +inf, even if not precisely.

Only if +inf is in the range of representable values, which, as already noted, 
is problematic.

This is a bit different from non-IEEE math like VAX, that doesn't have
infinities.

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



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

RE: r254574 - PR17381: Treat undefined behavior during expression evaluation as an unmodeled

[Robinson, Paul via cfe-commits]

| Oh right, sorry, off-by-one error when evaluating that by hand; I got 
0x7fff (which is also a NaN, 0x7fc0 is not the only NaN).
No worries.

| http://llvm.org/docs/LangRef.html#uitofp-to-instruction is clear that you get 
an undefined result for overflow currently.
Other parts of the LangRef are comfortable talking about infinities, e.g. 
there's a way to write them as constants, and IEEE float is pervasive in this 
era, so it would seem consistent for uitofp to return +infinity for the 
overflow case.  I'm not the one to propose it, though. ☺
--paulr

From: meta...@gmail.com [mailto:meta...@gmail.com] On Behalf Of Richard Smith
Sent: Wednesday, December 09, 2015 12:43 PM
To: Robinson, Paul
Cc: Joerg Sonnenberger; cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

On Wed, Dec 9, 2015 at 10:17 AM, Robinson, Paul via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
| And at runtime, on some targets, we use this:
|
|  
https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/builtins/floatuntisf.c
|
| ... which gives a NaN in this case.

I copied that function into a test program on Ubuntu, built with gcc, and it 
gives me +Infinity (0x7f80) not NaN (0x7fc0).

Oh right, sorry, off-by-one error when evaluating that by hand; I got 
0x7fff (which is also a NaN, 0x7fc0 is not the only NaN).

So, I think the question is, do we want to update LLVM to define the value of 
an out-of-range uitofp (and "fix" any targets that don't give +/- Inf for these 
conversions)? http://llvm.org/docs/LangRef.html#uitofp-to-instruction is clear 
that you get an undefined result for overflow currently.

In (AFAICS) all supported targets, for integer types supported by clang, there 
are only two ways to hit the overflow case:
 1) uint128 -> float
 2) uint64 or larger -> half

Case (1) goes through uitofp (which explicitly says the result is undefined at 
the moment), case (2) goes via @llvm.convert.to.fp16 (which says nothing about 
what happens in this case, but presumably it is defined). These are both 
phenomenally rare conversions, so adding (potential) extra cost to them to make 
them handle the out-of-range case correctly doesn't seem unreasonable.

--paulr

From: meta...@gmail.com<mailto:meta...@gmail.com> 
[mailto:meta...@gmail.com<mailto:meta...@gmail.com>] On Behalf Of Richard Smith
Sent: Tuesday, December 08, 2015 11:42 AM
To: Robinson, Paul
Cc: Joerg Sonnenberger; cfe-commits 
(cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>)

Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

On Tue, Dec 8, 2015 at 11:18 AM, Richard Smith 
<rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> wrote:
On Tue, Dec 8, 2015 at 10:59 AM, Robinson, Paul 
<paul_robin...@playstation.sony.com<mailto:paul_robin...@playstation.sony.com>> 
wrote:
Okay, I'll bite:  so what *does* UINT128_MAX actually convert to?

$ echo 'unsigned __int128 max = -1; float f = max;' | ~/clang-8/build/bin/clang 
-x c++ - -emit-llvm -S -o - -O3 | grep @f
@f = global float undef, align 4

And at runtime, on some targets, we use this:

  https://llvm.org/svn/llvm-project/compiler-rt/trunk/lib/builtins/floatuntisf.c

... which gives a NaN in this case.

From: cfe-commits 
[mailto:cfe-commits-boun...@lists.llvm.org<mailto:cfe-commits-boun...@lists.llvm.org>]
 On Behalf Of Richard Smith via cfe-commits
Sent: Tuesday, December 08, 2015 10:52 AM
To: Joerg Sonnenberger; cfe-commits
Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

On Tue, Dec 8, 2015 at 2:13 AM, Joerg Sonnenberger via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
On Mon, Dec 07, 2015 at 01:32:14PM -0800, Richard Smith via cfe-commits wrote:
> C11 6.3.1.5/1<http://6.3.1.5/1>: "If the value being converted is outside the 
> range of values
> that can be represented, the behavior is undefined."

The value of 1e100 can be represented as +inf, even if not precisely.

Only if +inf is in the range of representable values, which, as already noted, 
is problematic.

This is a bit different from non-IEEE math like VAX, that doesn't have
infinities.

Joerg
___
cfe-commits mailing list
cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits




___
cfe-commits mailing list
cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

| Types are a bit more vague (as to whether omitting unreferenced types is 
supported by the standard) DWARF 4 just says "Structure, union, and class types 
are represented by debugging information entries ...".

There's some expansion of the "permissive" discussion in the works for DWARF 5. 
 In essence, DWARF doesn't tell you _what_ to describe, but if you describe 
something, you do it _how_ the spec says.  So, omitting unused types and 
function declarations, or lexical blocks with no containing declarations, or 
even things like inlined subroutines, etc. etc. is all kosher, while things 
like the template parameter DIEs and the artificial import of anonymous 
namespaces are actually required.

| Any size numbers for this change?
I got in the neighborhood of 1% (just under, IIRC) of the sum of .debug_* 
sections for a self-build of Clang.

| In any case, it seems like it might make sense for you to upstream your 
template naming change and put it under the PS4 debugger tuning option, and put 
this change there too, once the motivation for it is in-tree. At that point, 
while I'd be curious about the size tradeoff, it'd be essentially academic

Exposing tuning up through Clang is actually very nearly at the top of my list 
now.
--paulr

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

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

Maybe we are being too pedantic about the names.  I'll have to go back and look 
in detail at why we decided to do that.

In any case, arguably 5.5.8 (Class Template Instantiations) 1 only applies to 
definitions of a type, not declarations. ("Each formal parameterized type 
declaration appearing in the template definition is represented by a debugging 
information entry with the tag DW_TAG_template_type_parameter")
Not so fast… It's a template definition of a type declaration.  DWARF 5 is less 
ambiguous about this IMO, although you are actually very good at finding the 
ambiguities!  The relevant text in DWARF 5 current draft is: "A debugging 
information entry that represents a template instantiation will contain child 
entries describing the actual template parameters."  Are you willing to argue 
that this type declaration is not an instantiation?  (If not, what is it?)

Why would that clause apply to attributes any less than it applies to DIEs?
It does apply equally.  However, nearly all attribute descriptions are 
specified as "may have" and therefore can be omitted freely without being 
non-conforming.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Wednesday, December 09, 2015 11:28 AM
To: Robinson, Paul
Cc: Marshall, Peter; llvm-dev; cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Wed, Dec 9, 2015 at 11:11 AM, Robinson, Paul 
> 
wrote:
Actually no, we prefer to have the original typedef names in the instantiation 
name, for source fidelity.

Then perhaps you should keep this change in your tree too - since that's where 
the need is?

  "Name as it is in the source" or something reasonably close.  Unwrapping 
typedefs is going too far.

Yet this isn't the choice upstream in Clang or GCC. I don't know about other 
DWARF generators, but it seems your interpretation isn't the way some other 
people/implementers are reading the DWARF spec.

[This seems like it would present a multitude of challenges to any DWARF 
debugger dealing with this kind of debug info - it'd have to know far more 
about the rules of the C++ language (which you've previously argued in favor of 
avoiding) to perform a variety of operations if the types don't match up fairly 
trivially.]

In any case, arguably 5.5.8 (Class Template Instantiations) 1 only applies to 
definitions of a type, not declarations. ("Each formal parameterized type 
declaration appearing in the template definition is represented by a debugging 
information entry with the tag DW_TAG_template_type_parameter") which, I agree, 
seems like a bug in the spec to not /allow/ them on declarations, but I'd 
equally argue requiring them would seem too narrow to me.

Re. "looseness" of the DWARF spec, it is not so loose as you like to think.  
Attributes tend to be fairly optional or can be used "in novel ways" but the 
DIEs and their relationships are not like that.  "Where this specification 
provides a means for describing the source language, implementors are expected 
to adhere to that specification."

Why would that clause apply to attributes any less than it applies to DIEs? It 
seems like a fairly broad statement.

I forget whether we already discussed it - but do you have any size data 
(preferably/possibly from a fission build or otherwise measurement of "just the 
debug info" not the whole binary) on, for example, a clang selfhost?

- Dave

--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Wednesday, December 09, 2015 10:49 AM
To: Robinson, Paul
Cc: Marshall, Peter; llvm-dev; cfe-commits 
(cfe-commits@lists.llvm.org)

Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Wed, Dec 9, 2015 at 10:40 AM, Robinson, Paul 
> 
wrote:
That doesn't seem to be the DWARF I'm seeing from Clang (& it'd be surprising 
if we used the typedef (or otherwise non-canonical) name in the class name):

Finally getting back to this…..  Ha.  We don't unwrap the typedefs ("name as it 
is in the source"), while the upstream compiler does.

Yeah, I imagine you'd want to fix that as I expect it would cause you other 
problems, no? (or is there some reason you have this change to the compiler? I 
imagine it'd be hard to have that divergence by accident?)

Providing the template-parameter DIEs is still the correct thing to do per the 
DWARF
spec.

I still don't agree that the DWARF we produce here is incorrect (the DWARF spec 
is pretty loose on "correctness" of DWARF). If there's some practical 
problem/use case it'd be useful to understand it so we make sure we're fixing 
it the right way.

- Dave

--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Friday, 

RE: r254574 - PR17381: Treat undefined behavior during expression evaluation as an unmodeled

[Robinson, Paul via cfe-commits]

| I've amended this change to permit constant-foldable UB in C variable 
initializers again in r254992.
Works for us, thanks!

| | (2) Shouldn't it diagnose each bad expression in an initializer?
| That change would be unrelated to the one at hand -- this is the way we've 
always behaved.
If it's consistent with past practice, that's fine.
Thanks,
--paulr

From: meta...@gmail.com [mailto:meta...@gmail.com] On Behalf Of Richard Smith
Sent: Monday, December 07, 2015 7:25 PM
To: Robinson, Paul
Cc: cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

I've amended this change to permit constant-foldable UB in C variable 
initializers again in r254992.

On Mon, Dec 7, 2015 at 6:45 PM, Robinson, Paul via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Two more questions:

(1) Commit message implied this is only for C, but I see it with C++11
(but not C++03).
$ cat t.cpp
enum { foo = 123456 * 234567 };
$ clang -c t.cpp -std=c++03
$ clang -c t.cpp -std=c++11
t.cpp:1:14: error: expression is not an integral constant expression

That is the behavior required by the C++11 standard, but we used to accept GNU 
constant folding here as an extension.



(2) Shouldn't it diagnose each bad expression in an initializer?
I see the error only for the first such expression.

$ cat t.c
int foo[2] = { 123456 * 234567, 654321 * 765432 };
$ clang -c t.c
t.c:1:23: error: initializer element is not a compile-time constant
int foo[2] = { 123456 * 234567, 654321 * 765432 };
   ~~~^~~~

That change would be unrelated to the one at hand -- this is the way we've 
always behaved. Consider for instance:

  int n;
  int foo[2] = { n, n };

If you wanted to make us report the other ones, the relevant code is in 
Expr::isConstantInitializer and Sema::CheckForConstantInitializer.

1 error generated.

Thanks,
--paulr

> -Original Message-
> From: cfe-commits 
> [mailto:cfe-commits-boun...@lists.llvm.org<mailto:cfe-commits-boun...@lists.llvm.org>]
>  On Behalf Of
> Richard Smith via cfe-commits
> Sent: Wednesday, December 02, 2015 5:36 PM
> To: cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
> Subject: r254574 - PR17381: Treat undefined behavior during expression
> evaluation as an unmodeled
>
> Author: rsmith
> Date: Wed Dec  2 19:36:22 2015
> New Revision: 254574
>
> URL: http://llvm.org/viewvc/llvm-project?rev=254574=rev
> Log:
> PR17381: Treat undefined behavior during expression evaluation as an
> unmodeled
> side-effect, so that we don't allow speculative evaluation of such
> expressions
> during code generation.
>
> This caused a diagnostic quality regression, so fix constant expression
> diagnostics to prefer either the first "can't be constant folded"
> diagnostic or
> the first "not a constant expression" diagnostic depending on the kind of
> evaluation we're doing. This was always the intent, but didn't quite work
> correctly before.
>
> This results in certain initializers that used to be constant initializers
> to
> no longer be; in particular, things like:
>
>   float f = 1e100;
>
> are no longer accepted in C. This seems appropriate, as such constructs
> would
> lead to code being executed if sanitizers are enabled.
>
> Added:
> cfe/trunk/test/CodeGen/ubsan-conditional.c
> Modified:
> cfe/trunk/lib/AST/ExprConstant.cpp
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
> cfe/trunk/test/CodeGen/complex-init-list.c
> cfe/trunk/test/PCH/floating-literal.c
> cfe/trunk/test/Sema/const-eval.c
> cfe/trunk/test/Sema/integer-overflow.c
> cfe/trunk/test/Sema/switch-1.c
> cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
> cfe/trunk/test/SemaCXX/constexpr-printing.cpp
>
> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=254574=254573=254574&
> view=diff
> ==
> 
> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
> +++ cfe/trunk/lib/AST/ExprConstant.cpp Wed Dec  2 19:36:22 2015
> @@ -473,6 +473,10 @@ namespace {
>  /// notes attached to it will also be stored, otherwise they will not
> be.
>  bool HasActiveDiagnostic;
>
> +/// \brief Have we emitted a diagnostic explaining why we couldn't
> constant
> +/// fold (not just why it's not strictly a constant expression)?
> +bool HasFoldFailureDiagnostic;
> +
>  enum EvaluationMode {
>/// Evaluate as a constant expression. Stop if we find that the
> expression
>/// is not a consta

RE: r254574 - PR17381: Treat undefined behavior during expression evaluation as an unmodeled

[Robinson, Paul via cfe-commits]

Okay, I'll bite:  so what *does* UINT128_MAX actually convert to?

From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of 
Richard Smith via cfe-commits
Sent: Tuesday, December 08, 2015 10:52 AM
To: Joerg Sonnenberger; cfe-commits
Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

On Tue, Dec 8, 2015 at 2:13 AM, Joerg Sonnenberger via cfe-commits 
> wrote:
On Mon, Dec 07, 2015 at 01:32:14PM -0800, Richard Smith via cfe-commits wrote:
> C11 6.3.1.5/1: "If the value being converted is outside the 
> range of values
> that can be represented, the behavior is undefined."

The value of 1e100 can be represented as +inf, even if not precisely.

Only if +inf is in the range of representable values, which, as already noted, 
is problematic.

This is a bit different from non-IEEE math like VAX, that doesn't have
infinities.

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

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

RE: r254574 - PR17381: Treat undefined behavior during expression evaluation as an unmodeled

[Robinson, Paul via cfe-commits]

Two more questions:

(1) Commit message implied this is only for C, but I see it with C++11
(but not C++03).

$ cat t.cpp
enum { foo = 123456 * 234567 };
$ clang -c t.cpp -std=c++03
$ clang -c t.cpp -std=c++11
t.cpp:1:14: error: expression is not an integral constant expression


(2) Shouldn't it diagnose each bad expression in an initializer?
I see the error only for the first such expression.

$ cat t.c
int foo[2] = { 123456 * 234567, 654321 * 765432 };
$ clang -c t.c
t.c:1:23: error: initializer element is not a compile-time constant
int foo[2] = { 123456 * 234567, 654321 * 765432 };
   ~~~^~~~
1 error generated.

Thanks,
--paulr

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Richard Smith via cfe-commits
> Sent: Wednesday, December 02, 2015 5:36 PM
> To: cfe-commits@lists.llvm.org
> Subject: r254574 - PR17381: Treat undefined behavior during expression
> evaluation as an unmodeled
> 
> Author: rsmith
> Date: Wed Dec  2 19:36:22 2015
> New Revision: 254574
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=254574=rev
> Log:
> PR17381: Treat undefined behavior during expression evaluation as an
> unmodeled
> side-effect, so that we don't allow speculative evaluation of such
> expressions
> during code generation.
> 
> This caused a diagnostic quality regression, so fix constant expression
> diagnostics to prefer either the first "can't be constant folded"
> diagnostic or
> the first "not a constant expression" diagnostic depending on the kind of
> evaluation we're doing. This was always the intent, but didn't quite work
> correctly before.
> 
> This results in certain initializers that used to be constant initializers
> to
> no longer be; in particular, things like:
> 
>   float f = 1e100;
> 
> are no longer accepted in C. This seems appropriate, as such constructs
> would
> lead to code being executed if sanitizers are enabled.
> 
> Added:
> cfe/trunk/test/CodeGen/ubsan-conditional.c
> Modified:
> cfe/trunk/lib/AST/ExprConstant.cpp
> cfe/trunk/lib/Sema/SemaChecking.cpp
> cfe/trunk/test/CXX/expr/expr.const/p2-0x.cpp
> cfe/trunk/test/CodeGen/complex-init-list.c
> cfe/trunk/test/PCH/floating-literal.c
> cfe/trunk/test/Sema/const-eval.c
> cfe/trunk/test/Sema/integer-overflow.c
> cfe/trunk/test/Sema/switch-1.c
> cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
> cfe/trunk/test/SemaCXX/constexpr-printing.cpp
> 
> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=254574=254573=254574&
> view=diff
> ==
> 
> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
> +++ cfe/trunk/lib/AST/ExprConstant.cpp Wed Dec  2 19:36:22 2015
> @@ -473,6 +473,10 @@ namespace {
>  /// notes attached to it will also be stored, otherwise they will not
> be.
>  bool HasActiveDiagnostic;
> 
> +/// \brief Have we emitted a diagnostic explaining why we couldn't
> constant
> +/// fold (not just why it's not strictly a constant expression)?
> +bool HasFoldFailureDiagnostic;
> +
>  enum EvaluationMode {
>/// Evaluate as a constant expression. Stop if we find that the
> expression
>/// is not a constant expression.
> @@ -537,7 +541,7 @@ namespace {
>  BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
>  EvaluatingDecl((const ValueDecl *)nullptr),
>  EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
> -EvalMode(Mode) {}
> +HasFoldFailureDiagnostic(false), EvalMode(Mode) {}
> 
>  void setEvaluatingDecl(APValue::LValueBase Base, APValue ) {
>EvaluatingDecl = Base;
> @@ -597,7 +601,7 @@ namespace {
>  /// Diagnose that the evaluation cannot be folded.
>  OptionalDiagnostic Diag(SourceLocation Loc, diag::kind DiagId
>= diag::note_invalid_subexpr_in_const_expr,
> -unsigned ExtraNotes = 0) {
> +unsigned ExtraNotes = 0, bool IsCCEDiag =
> false) {
>if (EvalStatus.Diag) {
>  // If we have a prior diagnostic, it will be noting that the
> expression
>  // isn't a constant expression. This diagnostic is more
> important,
> @@ -610,10 +614,9 @@ namespace {
>case EM_ConstantFold:
>case EM_IgnoreSideEffects:
>case EM_EvaluateForOverflow:
> -if (!EvalStatus.HasSideEffects)
> +if (!HasFoldFailureDiagnostic)
>break;
> -// We've had side-effects; we want the diagnostic from them,
> not
> -// some later problem.
> +// We've already failed to fold something. Keep that
> diagnostic.
>case EM_ConstantExpression:
>case EM_PotentialConstantExpression:
>case EM_ConstantExpressionUnevaluated:
> @@ -632,6 

RE: r254574 - PR17381: Treat undefined behavior during expression evaluation as an unmodeled

[Robinson, Paul via cfe-commits]

Explicitly cc: cfe-commits *again*….

| C11 6.3.1.5/1: "If the value being converted is outside the 
range of values that can be represented, the behavior is undefined."

I think 5.2.4.2.2/5 says if infinities are representable, there are no values 
"outside" the floating-point range?
And 6.3.1.5/1 starts out with "if the value being converted can be represented 
exactly in the new type, it is unchanged."  I'd think that NaNs could be 
represented "exactly" (they can certainly be represented: 5.2.4.2.2/3) despite 
not having an actual numeric value.
I don't think 6.3.1.5/1 means to exclude NaN/infinities, given that 6.3.1.4/1 
explicitly refers to "finite value" wrt conversion to an integer type.
--paulr



From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of 
Richard Smith via cfe-commits
Sent: Monday, December 07, 2015 1:32 PM
To: Joerg Sonnenberger; cfe-commits
Subject: Re: r254574 - PR17381: Treat undefined behavior during expression 
evaluation as an unmodeled

On Mon, Dec 7, 2015 at 7:25 AM, Joerg Sonnenberger via cfe-commits 
> wrote:
On Thu, Dec 03, 2015 at 01:36:22AM -, Richard Smith via cfe-commits wrote:
> Author: rsmith
> Date: Wed Dec  2 19:36:22 2015
> New Revision: 254574
>
> URL: http://llvm.org/viewvc/llvm-project?rev=254574=rev
> Log:
> PR17381: Treat undefined behavior during expression evaluation as an unmodeled
> side-effect, so that we don't allow speculative evaluation of such expressions
> during code generation.
>
> This caused a diagnostic quality regression, so fix constant expression
> diagnostics to prefer either the first "can't be constant folded" diagnostic 
> or
> the first "not a constant expression" diagnostic depending on the kind of
> evaluation we're doing. This was always the intent, but didn't quite work
> correctly before.
>
> This results in certain initializers that used to be constant initializers to
> no longer be; in particular, things like:
>
>   float f = 1e100;
>
> are no longer accepted in C. This seems appropriate, as such constructs would
> lead to code being executed if sanitizers are enabled.

This leads to some pretty annoying regressions as it now seems to be
impossible to use NaN or infinites as constant initializers. Expressions
like 0.0 / 0.0, 1.0 / 0.0 and -1.0 / 0.0 are perfectly well defined
under normal IEEE rules, so they shouldn't be rejected.

Well, we have a problem. The evaluation semantics of these expressions requires 
code to execute in some build modes (in particular, with sanitizers enabled), 
and thus has a side-effect.

I'm inclined to relax the restriction added in this change for the specific 
case of global variables in C, since (as you say) there is a fair amount of 
code using divide-by-zero as a "portable" way of generating an inf or nan.

Worse, it seems
even using __builtin_nan() for example doesn't work.

__builtin_nan() works fine for me, can you provide a testcase?

I'm not even sure about the example given in the commit message, how
exactly is that undefined behavior?

C11 6.3.1.5/1: "If the value being converted is outside the 
range of values that can be represented, the behavior is undefined."

We also have C11 6.6/4: "Each constant expression shall evaluate to a constant 
that is in the range of representable values for its type."
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: r254262 - [X86][SSE2] Added SSE2 IR + assembly codegen builtin tests

[Robinson, Paul via cfe-commits]

Resending because I forgot to explicitly CC the list AGAIN. :-P

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Simon Pilgrim via cfe-commits
> Sent: Sunday, November 29, 2015 12:23 PM
> To: cfe-commits@lists.llvm.org
> Subject: r254262 - [X86][SSE2] Added SSE2 IR + assembly codegen builtin
> tests
> 
> Author: rksimon
> Date: Sun Nov 29 14:23:00 2015
> New Revision: 254262
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=254262=rev
> Log:
> [X86][SSE2] Added SSE2 IR + assembly codegen builtin tests
> 
> Improved tests as discussed in PR24580
> 
> Added:
> cfe/trunk/test/CodeGen/sse2-builtins.c
> 
> Added: cfe/trunk/test/CodeGen/sse2-builtins.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/sse2-
> builtins.c?rev=254262=auto
> ==
> 
> --- cfe/trunk/test/CodeGen/sse2-builtins.c (added)
> +++ cfe/trunk/test/CodeGen/sse2-builtins.c Sun Nov 29 14:23:00 2015
> @@ -0,0 +1,1656 @@
> +// REQUIRES: x86-registered-target
> +// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin -target-feature +sse2 -
> emit-llvm -o - -Werror | FileCheck %s --check-prefix=DAG
> +// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin -target-feature +sse2 -
> fno-signed-char -emit-llvm -o - -Werror | FileCheck %s --check-prefix=DAG
> +// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin -target-feature +sse2 -
> S -o - -Werror | FileCheck %s --check-prefix=ASM
> +// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin -target-feature +sse2 -
> fno-signed-char -S -o - -Werror | FileCheck %s --check-prefix=ASM
> +
> +// Don't include mm_malloc.h, it's system specific.
> +#define __MM_MALLOC_H
> +
> +#include 
> +
> +__m128i test_mm_add_epi8(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_add_epi8
> +  // DAG: add <16 x i8>

Because DAG is a special suffix for FileCheck, I'd prefer not to have
it as the prefix.  I mean, it should work; but in a FileCheck context, 
DAG actually means something and I'd like not to have any more excuses 
than usual to be confused about reading FileChecks.
Thanks,
--paulr

> +  //
> +  // ASM-LABEL: test_mm_add_epi8
> +  // ASM: paddb
> +  return _mm_add_epi8(A, B);
> +}
> +
> +__m128i test_mm_add_epi16(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_add_epi16
> +  // DAG: add <8 x i16>
> +  //
> +  // ASM-LABEL: test_mm_add_epi16
> +  // ASM: paddw
> +  return _mm_add_epi16(A, B);
> +}
> +
> +__m128i test_mm_add_epi32(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_add_epi32
> +  // DAG: add <4 x i32>
> +  //
> +  // ASM-LABEL: test_mm_add_epi32
> +  // ASM: paddd
> +  return _mm_add_epi32(A, B);
> +}
> +
> +__m128i test_mm_add_epi64(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_add_epi64
> +  // DAG: add <2 x i64>
> +  //
> +  // ASM-LABEL: test_mm_add_epi64
> +  // ASM: paddq
> +  return _mm_add_epi64(A, B);
> +}
> +
> +__m128d test_mm_add_pd(__m128d A, __m128d B) {
> +  // DAG-LABEL: test_mm_add_pd
> +  // DAG: fadd <2 x double>
> +  //
> +  // ASM-LABEL: test_mm_add_pd
> +  // ASM: addpd
> +  return _mm_add_pd(A, B);
> +}
> +
> +__m128d test_mm_add_sd(__m128d A, __m128d B) {
> +  // DAG-LABEL: test_mm_add_sd
> +  // DAG: fadd double
> +  //
> +  // ASM-LABEL: test_mm_add_sd
> +  // ASM: addsd
> +  return _mm_add_sd(A, B);
> +}
> +
> +__m128i test_mm_adds_epi8(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_adds_epi8
> +  // DAG: call <16 x i8> @llvm.x86.sse2.padds.b
> +  //
> +  // ASM-LABEL: test_mm_adds_epi8
> +  // ASM: paddsb
> +  return _mm_adds_epi8(A, B);
> +}
> +
> +__m128i test_mm_adds_epi16(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_adds_epi16
> +  // DAG: call <8 x i16> @llvm.x86.sse2.padds.w
> +  //
> +  // ASM-LABEL: test_mm_adds_epi16
> +  // ASM: paddsw
> +  return _mm_adds_epi16(A, B);
> +}
> +
> +__m128i test_mm_adds_epu8(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_adds_epu8
> +  // DAG: call <16 x i8> @llvm.x86.sse2.paddus.b
> +  //
> +  // ASM-LABEL: test_mm_adds_epu8
> +  // ASM: paddusb
> +  return _mm_adds_epu8(A, B);
> +}
> +
> +__m128i test_mm_adds_epu16(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_adds_epu16
> +  // DAG: call <8 x i16> @llvm.x86.sse2.paddus.w
> +  //
> +  // ASM-LABEL: test_mm_adds_epu16
> +  // ASM: paddusw
> +  return _mm_adds_epu16(A, B);
> +}
> +
> +__m128d test_mm_and_pd(__m128d A, __m128d B) {
> +  // DAG-LABEL: test_mm_and_pd
> +  // DAG: and <4 x i32>
> +  //
> +  // ASM-LABEL: test_mm_and_pd
> +  // ASM: pand
> +  return _mm_and_pd(A, B);
> +}
> +
> +__m128i test_mm_and_si128(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_and_si128
> +  // DAG: and <2 x i64>
> +  //
> +  // ASM-LABEL: test_mm_and_si128
> +  // ASM: andps
> +  return _mm_and_si128(A, B);
> +}
> +
> +__m128i test_mm_avg_epu8(__m128i A, __m128i B) {
> +  // DAG-LABEL: test_mm_avg_epu8
> +  // DAG: call <16 x i8> @llvm.x86.sse2.pavg.b
> +  //
> +  // ASM-LABEL: test_mm_avg_epu8
> +  // ASM: pavgb
> +  return 

RE: r252834 - Provide a frontend based error for always_inline functions that require

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Eric Christopher via cfe-commits
> Sent: Wednesday, November 11, 2015 4:44 PM
> To: cfe-commits@lists.llvm.org
> Subject: r252834 - Provide a frontend based error for always_inline
> functions that require
> 
> Author: echristo
> Date: Wed Nov 11 18:44:12 2015
> New Revision: 252834
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=252834=rev
> Log:
> Provide a frontend based error for always_inline functions that require
> target features that the caller function doesn't provide. This matches
> the existing backend failure to inline functions that don't have
> matching target features - and diagnoses earlier in the case of
> always_inline.
> 
> Fix up a few test cases that were, in fact, invalid if you tried
> to generate code from the backend with the specified target features
> and add a couple of tests to illustrate what's going on.
> 
> This should fix PR25246.
> 
> Added:
> cfe/trunk/test/CodeGen/target-features-error-2.c
> cfe/trunk/test/CodeGen/target-features-error.c
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/lib/CodeGen/CGExpr.cpp
> cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
> cfe/trunk/test/CodeGen/3dnow-builtins.c
> cfe/trunk/test/CodeGen/avx512vl-builtins.c
> 
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=252834
> =252833=252834=diff
> ==
> 
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Nov 11
> 18:44:12 2015
> @@ -431,6 +431,9 @@ def err_builtin_definition : Error<"defi
>  def err_arm_invalid_specialreg : Error<"invalid special register for
> builtin">;
>  def err_invalid_cpu_supports : Error<"invalid cpu feature string for
> builtin">;
>  def err_builtin_needs_feature : Error<"%0 needs target feature %1">;
> +def err_function_needs_feature
> +: Error<"function %0 and always_inline callee function %1 are
> required to "
> +"have matching target features">;
>  def warn_builtin_unknown : Warning<"use of unknown builtin %0">,
>InGroup, DefaultError;
>  def warn_dyn_class_memaccess : Warning<
> 
> Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=252834=252833=252834
> ew=diff
> ==
> 
> --- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExpr.cpp Wed Nov 11 18:44:12 2015
> @@ -3747,6 +3747,15 @@ RValue CodeGenFunction::EmitCall(QualTyp
>assert(CalleeType->isFunctionPointerType() &&
>   "Call must have function pointer type!");
> 
> +  if (const FunctionDecl *FD =
> dyn_cast_or_null(TargetDecl))
> +// If this isn't an always_inline function we can't guarantee that
> any
> +// function isn't being used correctly 

Uh... I wouldn't mind this not using no fewer negatives...
an "only if" kind of phrasing might help.
Thanks,
--paulr

> so only check if we have the
> +// attribute and a set of target attributes that might be different
> from
> +// our default.
> +if (TargetDecl->hasAttr() &&
> +TargetDecl->hasAttr())
> +  checkTargetFeatures(E, FD);
> +
>CalleeType = getContext().getCanonicalType(CalleeType);
> 
>const auto *FnType =
> 
> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=252834=252833=
> 252834=diff
> ==
> 
> --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Wed Nov 11 18:44:12 2015
> @@ -1843,7 +1843,8 @@ template void CGBuilderInserter  llvm::BasicBlock::iterator InsertPt) const;
>  #undef PreserveNames
> 
> -// Returns true if we have a valid set of target features.
> +// Emits an error if we don't have a valid set of target features for the
> +// called function.
>  void CodeGenFunction::checkTargetFeatures(const CallExpr *E,
>const FunctionDecl *TargetDecl)
> {
>// Early exit if this is an indirect call.
> @@ -1856,31 +1857,70 @@ void CodeGenFunction::checkTargetFeature
>if (!FD)
>  return;
> 
> +  // Grab the required features for the call. For a builtin this is
> listed in
> +  // the td file with the default cpu, for an always_inline function this
> is any
> +  // listed cpu and any listed features.
>unsigned BuiltinID = TargetDecl->getBuiltinID();
> -  const char *FeatureList =
> -  CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID);
> -
> -  if (!FeatureList || StringRef(FeatureList) 

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

| Why is matching by name insufficient/not correct?
I'm told we look at the mangled names in the ELF symbol table, demangle them, 
and look in the DWARF for the corresponding types.  Now, the mangled name (for 
predefined types in particular) provides a type description, not the 
name-as-emitted-by-Clang, and in fact the same type can have more than one name 
('const int' and 'int const' for a trivial example).  The name Clang provides 
in the DWARF does not necessarily match the name produced by the demangler; 
this makes name-matching way more trouble than you'd think.  We're not 
interested in teaching the debugger how to parse template instantiation names.
Having the template type parameter means we have an unambiguous description of 
the type, and can match it easily.

| including unreferenced entities fails source fidelity
I'll assume you meant to say _excluding_ unreferenced entities fails source 
fidelity, which is quite true, but there is a valid engineering tradeoff in 
that what the DWARF actually contains (or not, in the case of, say, unused 
function declarations or unreferenced class contents) represents one possible 
valid source that could have produced the same object.  (I'm curious why an 
unreferenced formal parameter of a function still gets described, if this is 
your argument for omitting template parameters.)
Omitting template parameters however is not the same as omitting unreferenced 
entities, because the template parameters *are* referenced—by the template 
instantiation itself; and, omitting them from the source does not produce a 
valid program.  Now, one of the 3 debuggers Clang explicitly supports (i.e. 
gdb) seems not to mind that they're missing, but the other two would benefit 
from having these things, and I would really like to have Clang produce these 
things.

Thanks,
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Monday, November 09, 2015 1:46 PM
To: Robinson, Paul
Cc: reviews+d14358+public+d3104135076f0...@reviews.llvm.org; cfe-commits 
(cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Thu, Nov 5, 2015 at 11:05 AM, Robinson, Paul 
> 
wrote:
| What was your primary motivation?
A similar concern to PR20455 from our own debugger.  It much helps matching up 
the forward declaration and definition to have the parameters properly 
specified.

Why is matching by name insufficient/not correct?


| maybe it's possible to remangle the template using just the string name
I have no idea what you're talking about here.

Looking at PR20455 you linked, LLDB isn't finding the right function because of 
mangling:


call to a function 'basic_string::operator[](int) 
const' ('_ZNK12basic_stringIc17char_traitsEixEi') that is not present in 
the target
It hasn't created the correct mangled name of operator[] - what I was saying is 
it might be possible to parse the template parameter from the pretty name, and 
use that to produce the mangled name. It /looks/ like GDB can manage this. 
Maybe only because we also include the mangled name of the member function? Not 
sure.

| | Choosing to emit a forward/incomplete declaration in the first place fails 
source fidelity,
| How so?
When the source has a full definition but Clang chooses to emit only the 
declaration, per CGDebugInfo.cpp/shouldOmitDefinition().

Sure, in the same way that including unreferenced entities fails source 
fidelity - all tradeoffs to reduce debug info size.

Though the behavior is visible in a simpler example that doesn't have that 
failing (& if your change goes in, the test case should probably be simplified 
like this):

template struct foo;
foo *f;

--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Thursday, November 05, 2015 12:10 AM
To: Robinson, Paul
Cc: 
reviews+d14358+public+d3104135076f0...@reviews.llvm.org;
 cfe-commits (cfe-commits@lists.llvm.org)

Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Wed, Nov 4, 2015 at 11:32 PM, Robinson, Paul 
> 
wrote:
Would citing PR20455 help?  It wasn't actually my primary motivation but it's 
not too far off.  Having the template parameters there lets you know what's 
going on in the DWARF, without having to fetch and parse the name string of 
every struct you come across.  Actually I'm not sure parsing the name string is 
unambiguous either; each parameter is either a typename, or an expression, but 
without the parameter DIEs you don't know which, a-priori.  (What does  
mean? Depends on whether you think it should be a type name or a value; you 
can't tell, syntactically, you have to do some 

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

| when/where/why are types acquired from the mangled names of ELF symbols, 
rather than from corresponding DWARF?

Pete, can you help me out here?  David seems to want an ironclad case for not 
being able to do something any other way, before he will let me put the 
template type parameters on the declaration of a template instantiation.  (He 
does not deny that doing so would be valid DWARF, only that it can't possibly 
be *useful* DWARF.)
Thanks,
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Monday, November 09, 2015 4:08 PM
To: Robinson, Paul
Cc: reviews+d14358+public+d3104135076f0...@reviews.llvm.org; cfe-commits 
(cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Mon, Nov 9, 2015 at 3:55 PM, Robinson, Paul 
> 
wrote:
| Why is matching by name insufficient/not correct?
I'm told we look at the mangled names in the ELF symbol table, demangle them, 
and look in the DWARF for the corresponding types.

Not quite sure I follow that - when/where/why are types acquired from the 
mangled names of ELF symbols, rather than from corresponding DWARF? (eg: the 
DWARF describing the type of a function's parameter?)

  Now, the mangled name (for predefined types in particular) provides a type 
description, not the name-as-emitted-by-Clang, and in fact the same type can 
have more than one name ('const int' and 'int const' for a trivial example).  
The name Clang provides in the DWARF does not necessarily match the name 
produced by the demangler; this makes name-matching way more trouble than you'd 
think.  We're not interested in teaching the debugger how to parse template 
instantiation names.
Having the template type parameter means we have an unambiguous description of 
the type, and can match it easily.

| including unreferenced entities fails source fidelity
I'll assume you meant to say _excluding_ unreferenced entities fails source 
fidelity,

Indeed

which is quite true, but there is a valid engineering tradeoff in that what the 
DWARF actually contains (or not, in the case of, say, unused function 
declarations or unreferenced class contents) represents one possible valid 
source that could have produced the same object.  (I'm curious why an 
unreferenced formal parameter of a function still gets described, if this is 
your argument for omitting template parameters.)

Omitting parameters would make the function description unusable for callers, 
for example - so there's some value in describing them so that a debugger can 
evaluate expressions involving calls to the function, yes?

Omitting template parameters however is not the same as omitting unreferenced 
entities, because the template parameters *are* referenced—by the template 
instantiation itself;

Not quite sure I follow that logic. It's quite possible to have unreferenced 
template parameters:

  template
  void f() { }

and, omitting them from the source does not produce a valid program.

Omitting the names still produces a valid program - though I'm not quite sure 
which omission you're referring to. (& even if we omit the names, we still 
describe the parameters - as we do for unused/unnamed function parameters)

  Now, one of the 3 debuggers Clang explicitly supports (i.e. gdb) seems not to 
mind that they're missing, but the other two would benefit from having these 
things, and I would really like to have Clang produce these things.

It sounds like the LLDB bug you cited is being treated as an LLDB bug, not a 
Clang one, for now. So I'm not sure it's relevant to justifying Clang changes 
just yet, unless they come back & suggest that they don't actually have enough 
information to implement the features they would like to implement.

& equally I'd like to understand the features that you'd like to build with 
this info that can't be built without it (as a minimum: features that GDB 
doesn't support, since any features GDB does support seem to be implementable 
with the current info Clang and GCC emit)

- David


Thanks,
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Monday, November 09, 2015 1:46 PM

To: Robinson, Paul
Cc: 
reviews+d14358+public+d3104135076f0...@reviews.llvm.org;
 cfe-commits (cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Thu, Nov 5, 2015 at 11:05 AM, Robinson, Paul 
> 
wrote:
| What was your primary motivation?
A similar concern to PR20455 from our own debugger.  It much helps matching up 
the forward declaration and definition to have the parameters properly 
specified.

Why is matching by name insufficient/not correct?


| maybe it's possible 

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

| What was your primary motivation?
A similar concern to PR20455 from our own debugger.  It much helps matching up 
the forward declaration and definition to have the parameters properly 
specified.

| maybe it's possible to remangle the template using just the string name
I have no idea what you're talking about here.
| | Choosing to emit a forward/incomplete declaration in the first place fails 
source fidelity,
| How so?
When the source has a full definition but Clang chooses to emit only the 
declaration, per CGDebugInfo.cpp/shouldOmitDefinition().
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Thursday, November 05, 2015 12:10 AM
To: Robinson, Paul
Cc: reviews+d14358+public+d3104135076f0...@reviews.llvm.org; cfe-commits 
(cfe-commits@lists.llvm.org)
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Wed, Nov 4, 2015 at 11:32 PM, Robinson, Paul 
> 
wrote:
Would citing PR20455 help?  It wasn't actually my primary motivation but it's 
not too far off.  Having the template parameters there lets you know what's 
going on in the DWARF, without having to fetch and parse the name string of 
every struct you come across.  Actually I'm not sure parsing the name string is 
unambiguous either; each parameter is either a typename, or an expression, but 
without the parameter DIEs you don't know which, a-priori.  (What does  
mean? Depends on whether you think it should be a type name or a value; you 
can't tell, syntactically, you have to do some lookups.  Ah, but if you had the 
parameter DIEs, you would Just Know.)

For LLDB's needs, I'm not sure it's sufficient either - but I wouldn't mind an 
answer before we use it as the basis for this change (it sounds like maybe it's 
possible to remangle the template using just the string name, rather than 
needing an explicit representation of the parameters)

What was your primary motivation?

 Choosing to emit a forward/incomplete declaration in the first place fails 
source fidelity,

How so? You might have only a template declaration (template struct 
foo; foo *f;) or you may've only instantiated the declaration (the C++ 
language requires you to instantiate or avoid instantiating certain things in 
certain places, so in some contexts you /only/ have an instantiated 
declaration, not a definition)

but it is a practical engineering tradeoff of compile/link performance against 
utility; and, after all, the source *could* have been written that way, with no 
semantic difference.  But, if we're going to emit a white-lie incomplete 
declaration, we should do so correctly.

Again, "correct" in DWARF is a fairly nebulous concept.

--paulr

P.S. We should talk about this forward-declaration tactic wrt LTO sometime.  I 
have a case where a nested class got forward-declared; it's entirely 
conceivable that the outer class with the inner forward-declared class would 
end up being picked by LTO, leaving the user with no debug info for the inner 
class contents.

I believe this Just Works(tm). The things that can vary per-insntance of a type 
(implicit special members, member template implicit specializations, and nested 
types*) are not added to the type's child list, but they reference the child as 
their parent. So they continue to apply no matter which instance of the type is 
picked for uniquing (because of the name-based referencing, so the nested type 
definition just says "my parent is _Zfoo" and whatever _Zfoo we end up picking 
in the LTO linking/metadata deduplication will serve that role just fine)

* we could just do a better job of modelling nested types (& other non-globally 
scoped types) in a way that more closely models the source by emitting a 
declaration where they were declared, and a definition where they are defined 
(with the usual DW_AT_specification to wire them up)


From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Wednesday, November 04, 2015 8:30 PM
To: 
reviews+d14358+public+d3104135076f0...@reviews.llvm.org;
 Robinson, Paul
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Wed, Nov 4, 2015 at 5:53 PM, Paul Robinson via cfe-commits 
> wrote:
probinson added a comment.

GCC 4.8.4 on Linux doesn't produce these, but DWARF 4 section 5.5.8 says a 
class template instantiation is just like the equivalent non-template class 
entry, with the exception of the template parameter entries.  I read that as 
meaning an incomplete description (i.e. with DW_AT_declaration) lets you omit 
all the other children, but not the template parameters.

As usual, I think it's pretty hard to argue that DWARF /requires/ anything 
(permissive & all that). And I'm not sure that having these is particularly 

RE: [PATCH] D14358: DWARF's forward decl of a template should have template parameters.

[Robinson, Paul via cfe-commits]

Would citing PR20455 help?  It wasn't actually my primary motivation but it's 
not too far off.  Having the template parameters there lets you know what's 
going on in the DWARF, without having to fetch and parse the name string of 
every struct you come across.  Actually I'm not sure parsing the name string is 
unambiguous either; each parameter is either a typename, or an expression, but 
without the parameter DIEs you don't know which, a-priori.  (What does  
mean? Depends on whether you think it should be a type name or a value; you 
can't tell, syntactically, you have to do some lookups.  Ah, but if you had the 
parameter DIEs, you would Just Know.)

Choosing to emit a forward/incomplete declaration in the first place fails 
source fidelity, but it is a practical engineering tradeoff of compile/link 
performance against utility; and, after all, the source *could* have been 
written that way, with no semantic difference.  But, if we're going to emit a 
white-lie incomplete declaration, we should do so correctly.
--paulr

P.S. We should talk about this forward-declaration tactic wrt LTO sometime.  I 
have a case where a nested class got forward-declared; it's entirely 
conceivable that the outer class with the inner forward-declared class would 
end up being picked by LTO, leaving the user with no debug info for the inner 
class contents.

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Wednesday, November 04, 2015 8:30 PM
To: reviews+d14358+public+d3104135076f0...@reviews.llvm.org; Robinson, Paul
Subject: Re: [PATCH] D14358: DWARF's forward decl of a template should have 
template parameters.



On Wed, Nov 4, 2015 at 5:53 PM, Paul Robinson via cfe-commits 
> wrote:
probinson added a comment.

GCC 4.8.4 on Linux doesn't produce these, but DWARF 4 section 5.5.8 says a 
class template instantiation is just like the equivalent non-template class 
entry, with the exception of the template parameter entries.  I read that as 
meaning an incomplete description (i.e. with DW_AT_declaration) lets you omit 
all the other children, but not the template parameters.

As usual, I think it's pretty hard to argue that DWARF /requires/ anything 
(permissive & all that). And I'm not sure that having these is particularly 
valuable/useful - what use do you have in mind for them?

Wouldn't hurt to have some size info about the cost here, though I don't 
imagine it's massive, it does open us up to emitting a whole slew of new types 
(the types the template is instantiated with, and anything that depends on - 
breaking/avoiding type edges can, in my experience, be quite beneficial (I 
described an example of this in my lightning talk last week)).


I don't think omitting the template DIEs was an intentional optimization, in 
the sense of being a decision separate from deciding to emit the 
incomplete/forward declaration in the first place.  They were just omitted 
because we were omitting everything, but everything turns out to be 
non-compliant.


http://reviews.llvm.org/D14358



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

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

RE: r250173 - Always pass a -dwarf-version argument to integrated as.

[Robinson, Paul via cfe-commits]

> -Original Message-
> From: cfe-commits [mailto:cfe-commits-boun...@lists.llvm.org] On Behalf Of
> Douglas Katzman via cfe-commits
> Sent: Tuesday, October 13, 2015 9:23 AM
> To: cfe-commits@lists.llvm.org
> Subject: r250173 - Always pass a -dwarf-version argument to integrated as.
> 
> Author: dougk
> Date: Tue Oct 13 11:22:51 2015
> New Revision: 250173
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=250173=rev
> Log:
> Always pass a -dwarf-version argument to integrated as.
> 
> This removes the default of 3 hidden in the assembler previously.
> 
> Fixes breakage caused by r249655, reported by vsukharev.
> 
> Added:
> cfe/trunk/test/Driver/as-default-dwarf.s
> Modified:
> cfe/trunk/lib/Driver/Tools.cpp
> cfe/trunk/tools/driver/cc1as_main.cpp
> 
> Modified: cfe/trunk/lib/Driver/Tools.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/lib/Driver/Tools.cpp?rev=250173=250172=250173
> =diff
> ==
> 
> --- cfe/trunk/lib/Driver/Tools.cpp (original)
> +++ cfe/trunk/lib/Driver/Tools.cpp Tue Oct 13 11:22:51 2015
> @@ -5632,10 +5632,11 @@ void ClangAs::ConstructJob(Compilation &
>  if (Arg *A = Args.getLastArg(options::OPT_g_Group)) {
>WantDebug = !A->getOption().matches(options::OPT_g0);
>if (WantDebug) {
> -if ((DwarfVersion = DwarfVersionNum(A->getSpelling())) == 0)
> -  DwarfVersion = getToolChain().GetDefaultDwarfVersion();
> +DwarfVersion = DwarfVersionNum(A->getSpelling());
>}

The braces can be removed now that it's a one-liner 'then' clause.
--paulr

>  }
> +if (DwarfVersion == 0)
> +  DwarfVersion = getToolChain().GetDefaultDwarfVersion();
>  RenderDebugEnablingArgs(Args, CmdArgs,
>  (WantDebug ? CodeGenOptions::LimitedDebugInfo
> : CodeGenOptions::NoDebugInfo),
> 
> Added: cfe/trunk/test/Driver/as-default-dwarf.s
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/as-default-
> dwarf.s?rev=250173=auto
> ==
> 
> --- cfe/trunk/test/Driver/as-default-dwarf.s (added)
> +++ cfe/trunk/test/Driver/as-default-dwarf.s Tue Oct 13 11:22:51 2015
> @@ -0,0 +1,15 @@
> +@ REQUIRES: arm-registered-target
> +@ RUN: %clang --target=armv8a--linux-gnueabi -c %s -o %t
> +@ RUN: llvm-objdump -t %t | FileCheck %s
> +.text
> +.type   foo,%function
> +foo:
> +.fnstart
> +.cfi_startproc
> +
> +.Ltmp2:
> +.size   foo, .Ltmp2-foo
> +.cfi_endproc
> +.fnend
> +.cfi_sections .debug_frame
> +@ CHECK: foo
> 
> Modified: cfe/trunk/tools/driver/cc1as_main.cpp
> URL: http://llvm.org/viewvc/llvm-
> project/cfe/trunk/tools/driver/cc1as_main.cpp?rev=250173=250172=2501
> 73=diff
> ==
> 
> --- cfe/trunk/tools/driver/cc1as_main.cpp (original)
> +++ cfe/trunk/tools/driver/cc1as_main.cpp Tue Oct 13 11:22:51 2015
> @@ -144,7 +144,7 @@ public:
>  RelaxAll = 0;
>  NoExecStack = 0;
>  FatalWarnings = 0;
> -DwarfVersion = 3;
> +DwarfVersion = 0;
>}
> 
>static bool CreateFromArgs(AssemblerInvocation ,
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D12624: Top-level anonymous namespaces are missing import DW_TAG_imported_module and nested anonymous namespaces are not

[Robinson, Paul via cfe-commits]

This seems pretty fine-grained for a CodeGenOpt (not that I've looked there, 
perhaps there are examples of similarly fine grained things already there?)- 
I'm curious to understand the preference towards that rather than perhaps the 
more general "Debugger tuning" sort of thing Paul's implemented/could be pushed 
up here.

On the LLVM side, debugger tuning is basically a way to package up settings for 
a variety of rather specific flags. Then the individual flags control their 
respective fine-grained behaviors.  This approach was very clearly favored 
during the whole "what is tuning" design discussion.

So, having an "emit explicit import" flag follows that same design decision and 
makes rather more sense than peppering IRGen with tuning or triple checks.  
Whether the flag goes in CodeGenOpt or somewhere else is a separate question.  
There are other debug-related flags in there, but if they want to be factored 
out into their own DebugInfoOpt that's probably a separate topic/patch.
--paulr

From: David Blaikie [mailto:dblai...@gmail.com]
Sent: Tuesday, September 08, 2015 7:36 PM
To: reviews+d12624+public+25876849b7c59...@reviews.llvm.org; Richard Smith
Cc: Romanova, Katya; Eric Christopher; Robinson, Paul; Adrian Prantl; 
cfe-commits
Subject: Re: [PATCH] D12624: Top-level anonymous namespaces are missing import 
DW_TAG_imported_module and nested anonymous namespaces are not



On Tue, Sep 8, 2015 at 3:51 PM, Richard Smith via cfe-commits 
> wrote:
rsmith added inline comments.


Comment at: lib/CodeGen/CGDebugInfo.cpp:3263-3264
@@ +3262,4 @@
+  const NamespaceDecl *NSDecl = UD.getNominatedNamespace();
+  if (!NSDecl->isAnonymousNamespace() ||
+  CGM.getTarget().getTriple().isPS4CPU()) {
+DBuilder.createImportedModule(

probinson wrote:
> rsmith wrote:
> > I think we should do this unconditionally, to better match the source 
> > language semantics, but I'm curious what David, Eric, and other folks on 
> > the DWARF side think.
> David (in previous discussions and review comments) has said he thinks it is 
> unnecessary as the debugger already must know so much about C++ to get 
> various things right, it might as well know that it has to implicitly import 
> the anonymous namespace contents.  One example debugger UI allows the user to 
> type source-like syntax, and requires the debugger to apply (for example) C++ 
> parameter-type matching rules to distinguish between overloaded functions.  
> Compared to this, implicit imports are child's play.
>
> I believe Eric agrees with David; I don't remember whether Adrian said 
> anything in the previous iterations of this patch.
>
> I believe the explicit (although artificial) import is a good thing, because 
> it matches the source language semantics.  I find an important distinction 
> between "which declarations are available in this scope" and "how does the 
> user disambiguate declarations in this scope."  As a counterpart to the above 
> debugger UI example, I postulate a GUI drop-down list of symbols available 
> in-scope; this UI needs to know nothing about language semantics and 
> automatic imports, if the DWARF provides the correct explicit import.  This 
> suggests to me that the DWARF should provide it.
>
> There's also the piddly detail that debuggers are not the only consumers of 
> DWARF information, and presenting the DWARF in a more source-language-neutral 
> way (i.e., with the explicit artificial import) could be beneficial for those 
> other consumers, who might not necessarily want to learn language-specific 
> scoping rules.
>
> No debugger will be thrown for a loop if it sees the explicit import; however 
> for some debuggers it would be redundant (because they implicitly import the 
> anonymous namespace already).  There is a pretty trivial space savings if 
> it's omitted.
>
> Katya has mentioned the GCC and ICC precedent; in fairness I will say GCC 
> didn't used to emit this, and GDB tolerated that.
>
> Note that the DWARF standard does not tell us what to do; it merely tells us 
> how to emit the import, if we want to emit one.  Whether we want to emit one 
> is up to us.
>
I've chatted to David about this offline, and he said largely similar things. 
It seems that different DWARF consumers will want and expect different things 
here, so (sadly) we should do different things depending on who we think will 
be consuming the DWARF.

I'm fine keeping this conditional, but I don't think IR generation should be 
making this decision based on the triple, so I'd prefer it was phrased in a 
different way: add a CodeGenOpt for whether to emit imports for anonymous 
namespaces, and enable it for PS4 targets from the frontend.

This seems pretty fine-grained for a CodeGenOpt (not that I've looked there, 
perhaps there are examples of similarly fine grained things already there?)- 
I'm curious to understand the preference towards that