[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

probinson added a comment. In https://reviews.llvm.org/D37038#89, @aprantl wrote: > This may have gotten lost earlier: Would it be possible to instruct > CloneFunction to not clone any temporary MDNodes via one of the flags that > are passed to the ValueMapper? I did look at that, sorry

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

probinson added a comment. In https://reviews.llvm.org/D37038#89, @aprantl wrote: > This may have gotten lost earlier: Would it be possible to instruct > CloneFunction to not clone any temporary MDNodes via one of the flags that > are passed to the ValueMapper? I tried that. Basically,

[PATCH] D37038: Replace temp MD nodes with unique/distinct before cloning

probinson added a comment. I now think the tactic to pursue is making sure the containing DICompositeType(s) for the method have complete descriptions, and RAUW the temporary nodes, prior to calling CloneFunction. Actually wading around in the MDNodes, Types, and Decls to accomplish that resul

[PATCH] D37604: Disable debuginfo-tests for non-native configurations

probinson created this revision. This requires the host triple to match the default target triple, in order to run debuginfo-tests. It's possible this is too restrictive, but offhand it seems like a reasonable starting point. https://reviews.llvm.org/D37604 Files: lit.local.cfg Index: lit

[PATCH] D37604: Disable debuginfo-tests for non-native configurations

probinson added a comment. The new file goes in the debuginfo-tests directory. It's a separate project so that's probably not obvious from the diff. https://reviews.llvm.org/D37604 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lis

[PATCH] D37604: Disable debuginfo-tests for non-native configurations

probinson closed this revision. probinson added a comment. r312803. Forgot to put the tag in the commit message https://reviews.llvm.org/D37604 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D37604: Disable debuginfo-tests for non-native configurations

probinson added a comment. In https://reviews.llvm.org/D37604#864187, @aprantl wrote: > This seems reasonable to me, thanks! > When you commit this, could you please double-check that the tests are still > running on the green dragon builders? I'll also keep an eye on them. I was able to goog

[PATCH] D62850: [X86] Fix builtins-x86.c test where it wasn't using immediates. NFC

probinson added a comment. Is the compiler missing a check that these parameters are immediates? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62850/new/ https://reviews.llvm.org/D62850 ___ cfe-commits

[PATCH] D58033: Add option for emitting dbg info for call site parameters

probinson added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:755 + (Arch == llvm::Triple::x86 || Arch == llvm::Triple::x86_64)) +Opts.EnableDebugEntryValues = Args.hasArg(OPT_femit_debug_entry_values); + You want to disable entry-valu

[PATCH] D64780: Disallow most calling convention attributes on PS4.

probinson added reviewers: rnk, rjmccall. probinson added a comment. This has my blessing as PS4 code owner, but I'd like other eyes on it with respect to how we've gone about it. + rnk, rjmccall as the most likely suspects. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D58531: [clang] Specify type of pthread_create builtin

probinson added a reviewer: jdoerfert. probinson added a comment. We've started running into this too in building the PS4 system. +jdoerfert who added pthread_create to the builtin list. Looking at the patch, it seems straightforward enough although clearly needs clang-format-diff run over it.

[PATCH] D66328: [DebugInfo] Add debug location to dynamic atexit destructor

probinson added a comment. "debug-info-no-location.cpp" is an extremely generic name for a very specific case. "debug-info-atexit-stub.cpp" would be better. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66328/new/ https://reviews.llvm.org/D66328

[PATCH] D63894: [CXX] Exercise all paths through these tests

probinson created this revision. probinson added a reviewer: rsmith. Herald added a project: clang. Herald added a subscriber: cfe-commits. These 3 tests have dialect-based conditionals, but weren't running Clang with enough different dialects to actually enable those conditional sections. Repo

[PATCH] D63894: [CXX] Exercise all paths through these tests

probinson added a comment. Ping. This is pretty straightforward, the only question is whether we want to preserve these older-dialect tests or rip them out. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63894/new/ https://reviews.llvm.org/D63894 _

[PATCH] D63894: [CXX] Exercise all paths through these tests

probinson added a comment. In D63894#1574419 , @dblaikie wrote: > I would've assumed these conditionals were added by Sony folks for their > change in default dialect - that doesn't necessarily mean these tests are > needed upstream (the functionality ma

[PATCH] D63894: [CXX] Exercise all paths through these tests

probinson added a comment. In D63894#1576288 , @probinson wrote: > there is no reason to think those paths are tested elsewhere. Well that may be a tad strong. But I'm reluctant to remove them unless they are demonstrably tested elsewhere, and it's not

[PATCH] D63894: [CXX] Exercise all paths through these tests

probinson marked an inline comment as done. probinson added inline comments. Comment at: clang/test/SemaCXX/linkage2.cpp:3 +// RUN: %clang_cc1 -fsyntax-only -verify -Wno-c++11-extensions -Wno-local-type-template-args %s -std=gnu++98 // RUN: %clang_cc1 -fsyntax-only -verify -Wno

[PATCH] D63894: [CXX] Exercise all paths through these tests

This revision was automatically updated to reflect the committed changes. Closed by commit rL36: [CXX] Exercise all paths through these tests. (authored by probinson, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://revi

[PATCH] D60283: [DebugInfo] Don't emit checksums when compiling a preprocessed CPP

probinson added a comment. Minor stuff. This solution is surprisingly simple, the main question being (and I don't have an answer) whether increasing the size of PresumedLoc is okay. Comment at: lib/Basic/SourceManager.cpp:1460 +FID = FileID::get(0); // contents of fi

[PATCH] D60283: [DebugInfo] Don't emit checksums when compiling a preprocessed CPP

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60283/new/ https://reviews.llvm.org/D60283 ___ cfe-comm

[PATCH] D62202: Work around a Visual C++ bug

probinson created this revision. probinson added reviewers: aaron.ballman, rnk. Herald added a project: clang. Herald added a subscriber: cfe-commits. Put up for review because I'm not clear whether this should be considered a permanent fix, or if I should put some sort of comment here indicating

[PATCH] D62202: Work around a Visual C++ bug

probinson added a comment. To provide some missing details: The original source gets a bogus compile-time error with Visual Studio 2017, prior to version 15.8. I have 15.2, so I need this patch to build Clang. Our current minimum-version requirement (per CheckCompilerVersion.cmake) is Visual St

[PATCH] D62202: Work around a Visual C++ bug

probinson added a comment. In D62202#1510414 , @dblaikie wrote: > Technically this violates the LLVM style guide which says "make anonymous > namespaces as small as possible, and only use them for class declarations." > (preferring static for functions)

[PATCH] D62202: Work around a Visual C++ bug

probinson added a comment. The helper is passed as a template parameter to a class ctor, and that instantiated class calls the helper from operator(), so I suppose maybe enough indirection through lambdas but this seems kind of involved for getting my builds to work. Repository: rC Clang

[PATCH] D62202: Work around a Visual C++ bug

probinson added a comment. // FIXME: Change the following functions from anonymous namespace to static // after the minimum _MSC_VER >= 1915 (equivalent to Visual Studio version // of 15.8 or higher). Works around a bug in earlier versions. ? Also happy to include the hyperlink to the bug

[PATCH] D62202: Work around a Visual C++ bug

This revision was automatically updated to reflect the committed changes. Closed by commit rL361502: Work around a Visual C++ bug. (authored by probinson, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D62

[PATCH] D55712: [Driver][PS4] Do not implicitly link against asan or ubsan if -nostdlib or -nodefaultlibs on PS4.

probinson added a comment. LGTM but I will defer to @filcab as he is more familiar with the area. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55712/new/ https://reviews.llvm.org/D55712 ___ cfe-commits mailing list

[PATCH] D55712: [Driver][PS4] Do not implicitly link against asan or ubsan if -nostdlib or -nodefaultlibs on PS4.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. I missed that filcab had said ok internally. Go ahead Pierre. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55712/new/ https://reviews.llvm.org/D55712

[PATCH] D56393: [DebugInfo] Don't emit DW_AT_enum_class unless it's actually an 'enum class'.

probinson created this revision. probinson added reviewers: dblaikie, rnk, zturner. probinson added a project: debug-info. Herald added subscribers: cfe-commits, JDevlieghere, aprantl. The original fix for PR36168 would emit DW_AT_enum_class for both of these cases: enum Fixed : short { F1

[PATCH] D56393: [DebugInfo] Don't emit DW_AT_enum_class unless it's actually an 'enum class'.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rC350636: Don't emit DW_AT_enum_class unless it's actually an 'enum class'. (authored by probinson, committed by ). Changed

[PATCH] D55781: Make CC mangling conditional on the ABI version

probinson added a comment. Herald added a subscriber: mstorsjo. Sorry for not noticing this sooner. The TL;DR is, the current patch does not affect PS4, so go ahead; but see the long version for other considerations. First, the general point: The PS4 ABI does not fit neatly into the sequential

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

probinson added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:144 +// includes the full path. +if (SM.getFilename(IL).contains("UnifiedSource")) { + StringRef Name = SM.getFilename(SL); george.karpenk

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

probinson added inline comments. Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h:144 +// includes the full path. +if (SM.getFilename(IL).contains("UnifiedSource")) { + StringRef Name = SM.getFilename(SL); NoQ wrote: > N

[PATCH] D45839: [analyzer] Add support for WebKit "unified sources".

probinson added a comment. In https://reviews.llvm.org/D45839#1077258, @NoQ wrote: > Aha, ok, yeah, that sounds like a lot, thank you. I think i'll follow up with > a separate commit that will enable first-level-code-file-include analysis in > all files under an on-by-default `-analyzer-config`

[PATCH] D46139: Add template type and value parameter metadata nodes to template variable specializations.

probinson added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:3023 + templateParameters = parameterNodes.get(); + // Since we emit declarations (DW_AT_members) for static members, place the Naively it looks like it should be possible to put the loc

[PATCH] D46218: PR37275 packed attribute should not apply to base classes

probinson added a comment. In https://reviews.llvm.org/D46218#1081933, @rjmccall wrote: > I wonder if that was originally just an oversight that got turned into > official policy. Anyway, if it's the policy, it's what we have to do; LGTM. I think it's actually correct behavior. Why would an

[PATCH] D48989: -fdebug-prefix-map option for cc1as

probinson added a comment. In https://reviews.llvm.org/D48989#1153957, @starsid wrote: > In https://reviews.llvm.org/D48989#1153773, @compnerd wrote: > > > However, please add a test to ensure that the paths are mapped when > > invoking the assembler > > > I added the tests to check the mapping

[PATCH] D47172: [FileCheck] Add -allow-deprecated-dag-overlap to failing clang tests

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D47172 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D48989: -fdebug-prefix-map option for cc1as

This revision was automatically updated to reflect the committed changes. Closed by commit rC336685: Support -fdebug-prefix-map for assembler source (pass to cc1as). This (authored by probinson, committed by ). Changed prior to commit: https://reviews.llvm.org/D48989?vs=154669&id=154811#toc R

[PATCH] D46190: For a used declaration, mark any associated usings as referenced.

probinson added a comment. A bunch of style comments. Maybe try clang-format-diff. Comment at: include/clang/Sema/SemaInternal.h:91 Var->markUsed(SemaRef.Context); + SemaRef.MarkUsingReferenced(Var, Loc, /*CXXScopeSpec*=*/nullptr, RefExpr); } The comments

[PATCH] D68117: [DWARF-5] Support for C++11 defaulted, deleted member functions.

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1618 + return; +} else if (const auto Def = Method->getDefinition()) { + if (Def->isDefaulted()) { LLVM style is not to use 'else' after 'return'. Comm

[PATCH] D68117: [DWARF-5] Support for C++11 defaulted, deleted member functions.

probinson added inline comments. Comment at: llvm/include/llvm/IR/DebugInfoFlags.def:91 HANDLE_DISP_FLAG((1u << 8), MainSubprogram) +HANDLE_DISP_FLAG((1u << 9), NotDefaulted) +HANDLE_DISP_FLAG((1u << 10), DefaultedInClass) probinson wrote: > SouraVX wrote: > > a

[PATCH] D68117: [DWARF-5] Support for C++11 defaulted, deleted member functions.

probinson added a comment. We really do want to pack the four mutually exclusive cases into two bits. I have tried to give more explicit comments inline to explain how you would do this. It really should work fine, recognizing that the "not defaulted" case is not explicitly represented in the

[PATCH] D68117: [DWARF-5] Support for C++11 defaulted, deleted member functions.

probinson added a comment. In D68117#1709557 , @SouraVX wrote: > Hi David, > I did some digging about DW_AT_defaulted and stuff, not much of a success > but. Here's what I found -- http://dwarfstd.org/Issues.php?type=closed4 -- > here it;s still marke

[PATCH] D67723: [DebugInfo] Add option to disable inline line tables.

probinson added a comment. Apologies for missing this until now. Our email system keeps dropping stuff sent by Phabricator. FTR, since @rnk has mentioned my years-ago writings, what Sony has internally nowadays is a little different than what I said back then. We have an option spelled `-gno

[PATCH] D68117: [DWARF-5] Support for C++11 defaulted, deleted member functions.

probinson added a comment. In D68117#1710295 , @dblaikie wrote: > I think the existing feature's perhaps a bit misspecified - because where you > put the DEFAULTED_{in_class,out_of_class} would communicate that without > needing the two values - if you p

[PATCH] D68117: [DWARF-5] Support for C++11 defaulted, deleted member functions.

probinson added a comment. In D68117#1711714 , @dblaikie wrote: > In D68117#1711154 , @probinson wrote: > > > (@dblaikie Aside regarding noreturn, the original DWARF proposal was for a > > debugger wanting to know

[PATCH] D67723: [DebugInfo] Add option to disable inline line tables.

probinson added a comment. > These experiments are convincing me that, in general, line zero isn't that > helpful for DWARF consumers. If the goal is to get smooth stepping, we may > want to refocus on getting reliable is_stmt bits in the line table. If you mean, it's not useful for identifying

[PATCH] D69393: [RFC][DebugInfo] emit user specified address_space in dwarf

probinson added a comment. The DW_AT_address_class attribute is intended to be used for target architectures where a simple address value is ambiguous, and the debugger needs additional information to resolve the ambiguity. The canonical example is something like i386 with its segmented addres

[PATCH] D69393: [RFC][DebugInfo] emit user specified address_space in dwarf

probinson added a comment. In D69393#1720816 , @ast wrote: > > The address spaces envisioned by the Linux kernel appear to be more > > informational and not descriptive of hardware characteristics. > > From the kernel pov the `__user` and normal are two d

[PATCH] D80876: [clang] Default to windows response files when running on windows

probinson added inline comments. Comment at: clang/tools/driver/driver.cpp:391 +if (ClangCLMode || +llvm::Triple(llvm::sys::getProcessTriple()).isOSWindows()) + Tokenizer = &llvm::cl::TokenizeWindowsCommandLine; Testing the triple is a functional

[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

probinson added a comment. In D80242#2045362 , @nickdesaulniers wrote: > For C++, I'd imagine: > > class foo {}; > using my_foo = foo; > template > struct baz { > bar my_bar; > }; > > > but it seems that `g++` doesn't emit debug info the fo

[PATCH] D80876: [clang] Default to windows response files when running on windows

probinson added a comment. As a functional change it should definitely have a functional test. Let me double-check on how our stuff behaves, I may be mis-remembering. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80876/new/ https://reviews.llvm.org

[PATCH] D80876: [clang] Default to windows response files when running on windows

probinson added a comment. Indeed I am mis-remembering, sorry about that! We use gnu-style command-line options but we change RSPQuoting from Default to Windows; assuming getProcessTriple() is the host not the target, your change should have the same effect. I'll try removing our private patc

[PATCH] D80876: [clang] Default to windows response files when running on windows

probinson added a subscriber: rnk. probinson added a comment. I was about to add @rnk who I remember has been in Windows driver behavior discussions in the past; except he has cleverly left a note that he's away June-Sept. Oh well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D80876: [clang] Default to windows response files when running on windows

probinson added a comment. It looks like your patch will allow us to remove a private patch that has a similar effect. Incidentally if I apply this to an upstream checkout on Windows, I see clang/test/Driver/at_file.c fails, so you'd at least need to do something for that. (`UNSUPPORTED: system

[PATCH] D80876: [clang] Default to windows response files when running on windows

probinson added a comment. Re testing, you could copy clang/test/Driver/at_file_win.c, which has an explicit `--rsp-quoting=windows`; remove that option and make it `REQUIRES: system-windows`. That should do it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D77393: [X86] Fix implicit sign conversion warnings in X86 headers.

probinson added a comment. It looks like you're not actually interested in the compiled output, but just whether warnings occurred; in that case you'd be better off with `-verify -fsyntax-only` and `// expected-no-diagnostics`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77393/new/

[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

probinson added a subscriber: debug-info. probinson added a comment. +debug-info tag Needs a Driver test to show that the Right Thing gets passed to cc1. Comment at: clang/docs/CommandGuide/clang.rst:438 + + By default, Clang does not emit type information for types that are

[PATCH] D81970: [Clang][Driver] Remove gold linker support for PS4 toolchain

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM with one inline comment. Comment at: clang/lib/Driver/ToolChains/PS4CPU.cpp:154 const char *Exec = -#ifdef _WIN32 - Args.MakeArgString(ToolChain.GetProgram

[PATCH] D80242: [Clang] implement -fno-eliminate-unused-debug-types

probinson added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2155 + +Emit debug info for types that are unused but defined by the program. + I think this description goes counter to how other options are described, which basically is abo

[PATCH] D78024: [FileCheck] - Fix the false positive when -implicit-check-not is used with an unknown -check-prefix.

probinson added a comment. I'm just reading this review for the first time, and my thought was, why is this interacting with implicit-check-not? I think specifying a `--check-prefix=FOO` with no uses of FOO should be diagnosed. I can't say I recall the previous discussion in detail, but that's

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

probinson added a comment. Looks okay (one grammar nit), probably worth waiting for someone else to chime in. Comment at: clang/docs/ClangCommandLineReference.rst:2139 -Enable stack protectors for some functions vulnerable to stack smashing. This uses a loose heuristic whic

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

probinson added a comment. Whoa--that's the *help* text? Well, if that's the only documentation for options that there is, I guess it has to go there; but it seems pretty excessive for the (ideally concise) output of `clang --help`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D40948: Switch Clang's default C++ language target to C++14

probinson added a reviewer: rjmccall. probinson added a comment. +rjmccall for the codegen bits. Comment at: clang/test/SemaCXX/new-array-size-conv.cpp:1 -// RUN: %clang_cc1 -fsyntax-only -pedantic -verify %s +// RUN: %clang_cc1 -fsyntax-only -pedantic -verify -std=gnu++98 %s

[PATCH] D40948: Switch Clang's default C++ language target to C++14

probinson added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1733 // The PS4 uses C++11 as the default C++ standard. - if (T.isPS4()) -LangStd = LangStandard::lang_gnucxx11; - else -LangStd = LangStandard::lang_gnucxx98;

[PATCH] D40712: Add cc1 flag enabling the function stack size section that was added in r319430

probinson added a comment. The title says "Add cc1 option..." which to me implies a "cc1 only" option. What you're actually doing is adding a driver option. Please update the title. Comment at: test/Driver/stack-size-section.c:2 +// RUN: %clang -target x86_64-unknown %s -S -

[PATCH] D41044: Implementation of -fextend-lifetimes and -fextend-this-ptr to aid with debugging of optimized code

probinson added a comment. I should note we've had at least one request to make this specifiable per-function, which would mean defining an attribute to control the emission of the fake-use intrinsics. Doing the feature that way would be consistent with 'optnone'. https://reviews.llvm.org/D4

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

probinson added a comment. Philosophically, mangled names and DWARF information serve different purposes, and I don't think you will find one true solution where both of them can yield the same name that everyone will be happy with. Mangled names exist to provide unique and reproducible identi

[PATCH] D131820: [PS4][driver] make -fjmc work with LTO driver linking stage

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM with one comment on the test. Comment at: clang/test/Driver/ps4-ps5-linker-jmc.c:1 +// REQUIRES: x86-registered-target + This REQUIRES is not nece

[PATCH] D132950: Remove unnecessary `REQUIRES: x86-registered-target` from ps4/ps5 driver tests.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM. Checking the tests over by eye, it looks like all run the clang driver with `-###` or `-E` so only the driver itself is invoked, and no backend dependency exists. Repository:

[PATCH] D133191: Driver test: remove `REQUIRES: x86-registered-target` and set `--sysroot=""` to support clang with `DEFAULT_SYSROOT`.

probinson closed this revision. probinson added a comment. Closing this for @MaggieYi the commit message didn't cite the Phabricator review. https://github.com/llvm/llvm-project/commit/5de4d97a00b2a5d710892e96d77810784fd2cd5c Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

probinson added a reviewer: probinson. probinson added a comment. > Yeah, but it seems that it is not possible to use lld for PS4 It's correct that PS4 does not use lld. PS5 does use lld (under a different name). But, we don't use addLTOOptions() for either PS4 or PS5, so this patch doesn't af

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

probinson added a comment. I'm happy if @dblaikie is happy. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:515 +// +// And as @dblaikie noted, this solution is far from perfert, better to +// encode it into IR metadata, but this may not worth it, since looks

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:515 +// +// But note, this solution is far from perfert, better to encode it into IR +// metadata, but this may not worth it, since looks like aranges on the way T

[PATCH] D133092: [clang] fix generation of .debug_aranges with LTO

probinson added a comment. > we don't use addLTOOptions() for either PS4 or PS5, so this patch doesn't > affect (or help) them. I'm okay with saying we'll need to deal with that case > separately. FTR, I've raised an internal ticket about this, and someone from Sony will handle it. Repositor

[PATCH] D133875: [clang] fix generation of .debug_aranges with LTO (resubmit)

probinson added inline comments. Comment at: clang/test/Driver/debug-options-aranges.c:7 +// RUN: %clang -### -g -target x86_64-linux -flto=thin -fuse-ld=lld -gdwarf-aranges %s 2>&1 | FileCheck -check-prefix=PLUGIN_GARANGE %s +// GARANGE: -generate-arange-section +// PLUGIN_GARA

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

probinson added a comment. Seems like for an "auto"-returning function/lambda with a definition, the front-end should have deduced the return type, and so we should emit that in the DWARF, even if we end up emitting DWARF with both a declaration and a separate definition. I accept that a membe

[PATCH] D128934: [X86] Add RDPRU instruction

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG08e4fe6c6196: [X86] Add RDPRU instruction (authored by probinson). Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed pr

[PATCH] D122503: Remove dead code in driver parsing -gsimple-template-names= options

probinson added a subscriber: cfe-commits. probinson added a comment. +cfe-commits; that should have happened automatically? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122503/new/ https://reviews.llvm.org/D122503 ___ cfe-commits mailing li

[PATCH] D122503: Remove dead code in driver parsing -gsimple-template-names= options

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6aa039775891: Remove dead code in driver parsing -gsimple-template-names= options (authored by probinson). Herald added a project: clang. Repository

[PATCH] D130786: [clang-repl] Disable execution unittests on unsupported platforms.

probinson added a comment. If you're going to post a patch for review, you really should wait for someone to review it before you land it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130786/new/ https://reviews.llvm.org/D130786

[PATCH] D120175: [Driver] Re-run lld with --reproduce when it crashes

probinson added a comment. > lld has tests for --reproduce already. Even if I added an environment > variable to crash lld, the test would depend on lld being built, adding a big > dependency to clang's tests. Do we think that's worth it? There is now cross-project-tests which would seem to be

[PATCH] D120175: [Driver] Re-run lld with --reproduce when it crashes

probinson added a comment. > there didn't already exist a PS4 lit feature that I could mark as unsupported. ? I believe `UNSUPPORTED: ps4` should work, because UNSUPPORTED will look at the default target triple. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default dialect

probinson added a comment. What Aaron said. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131465/new/ https://reviews.llvm.org/D131465 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

probinson added a reviewer: jmorse. probinson added a subscriber: jmorse. probinson added a comment. + @jmorse who is better placed than I am to say whether this is what Sony would prefer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106084/new/

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

probinson added a comment. > (hence the renaming of "limited" a long time ago to "standalone-debug" to > create a policy/philosophy around what goes in each category). Sorry, what? I thought -fstandalone-debug meant FullDebugInfo, and AFAICT that's still how the driver handles it? Repository

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

probinson added a comment. > Wouldn't the current "limited" behavior have problems for this shared > libraries situation too? Sounds like in that case -fstandalone-debug should > be used. @jmorse am I remembering correctly, that we require dllimport-style annotations, so "limited" actually inc

[PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

probinson added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1636 +if (Opts.getDebugInfo() == codegenoptions::DebugInfoConstructor) + Opts.setDebugInfo(codegenoptions::LimitedDebugInfo); No... you want to check both options in

[PATCH] D111587: re-land: [clang] Fix absolute file paths with -fdebug-prefix-map

probinson added a comment. In D111587#3381369 , @keith wrote: > Fix tests with dwarf 6 Do you mean dwarf 5 here? There is no v6 yet. Comment at: clang/test/Modules/module-debuginfo-prefix.m:24 -// Dir should always be empty, but on

[PATCH] D111587: re-land: [clang] Fix absolute file paths with -fdebug-prefix-map

probinson added a comment. In D111587#3382834 , @keith wrote: > I actually mean dwarf 6, which appears to be partially implemented according > to https://lists.llvm.org/pipermail/llvm-dev/2020-April/141055.html > > I discovered the issue from the failed

[PATCH] D119040: Fix LookupTest where it was missing an assertion

probinson abandoned this revision. probinson added a comment. In D119040#3363406 , @kadircet wrote: > Thanks for the change, but the test is actually checking for rename in > presence of using-decls. I beleive https://reviews.llvm.org/D121103 is the > p

[PATCH] D111587: re-land: [clang] Fix absolute file paths with -fdebug-prefix-map

probinson added a comment. In D111587#3383684 , @keith wrote: > You're right it's version 5 not 4, maybe the issue is that some platforms > (like macOS) are defaulting to 4 intentionally for now? I guess I thought 6 > because passing 6 also reproduces,

[PATCH] D118850: [PS4] Make __BIGGEST_ALIGNMENT__ 32bytes

probinson accepted this revision. probinson added a comment. LGTM, sorry I missed this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118850/new/ https://reviews.llvm.org/D118850 ___ cfe-commits mailing

[PATCH] D121678: [pseudo] Split greatergreater token.

probinson added a comment. We had the same thing happen downstream. One of our guys speculates that because some allocations are "owned" by the returned TokenStream, but the returned TokenStream is a temporary, the lifetimes might not be sufficient and some deallocations can happen. If I chang

[PATCH] D123319: Change how we handle auto return types for lambda operator() to be consistent with gcc

probinson added a comment. In D123319#3437250 , @dblaikie wrote: > (@probinson as someone I've disagreed with about this before) > > Personally I think there's limited value in expressing 'auto' in DWARF at all > - we could omit function declarations if

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

probinson added a comment. > What's the DW_AT_type referring to? (why is there a type called ".str"?) I'd expect it to point to a base type, in this case character string. A typeless variable with a location would be pretty weird. But I agree the name/linkage_name would be unnecessary. Reposi

[PATCH] D119040: Fix LookupTest where it was missing an assertion

probinson created this revision. probinson added a reviewer: ioeric. probinson requested review of this revision. The preceding EXPECT_EQ was never executed; modifying the test input made that happen. Found by the Rotten Green Tests project. https://reviews.llvm.org/D119040 Files: clang/unit

[PATCH] D119040: Fix LookupTest where it was missing an assertion

probinson added a comment. By "the EXPECT_EQ was never executed" I mean, replacing it with `assert(false);` does not crash the test. The string `"a::b::Foo"` was never seen by the Visitor. Maybe this indicates some much more subtle, deeper problem; I don't know. This change does cause the EX

[PATCH] D118471: [clang][Lexer] Make raw and normal lexer behave the same for line comments

probinson added inline comments. Comment at: clang/unittests/Lex/LexerTest.cpp:654 + while (!L.LexFromRawLexer(T)) { +ASSERT_TRUE(!ToksView.empty()); +EXPECT_EQ(T.getKind(), ToksView.front().getKind()); @kadircet @sammccall It turns out this while loop

<    1   2   3   4   5   6   >