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

probinson added a comment. In D58033#1470260 , @djtodoro wrote: > @probinson @aprantl Thanks a lot for your comments! > > Let's clarify some things. I'm sorry about the confusion. > > Initial patch for the functionality can be restricted by this option

[PATCH] D61496: Fixed tests where grep was not matching the linefeed

probinson added a comment. Checked-in files should not have CRLF endings, in general (there are a very few exceptions, and these aren't among them). If the checked-in files have LF endings but your tool checks them out with CRLF then that is a problem with your config, or with the tool. Adding

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

probinson added a comment. In D60283#1480546 , @aganea wrote: > Thanks Paul, your solution is even better. I'll apply rL11 > locally - if everything's fine, do you > mind if I re-land it again? I suggest you do

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

[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] D58033: Add option for emitting dbg info for call site parameters

probinson added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:3402 +CmdArgs.push_back("-femit-param-entry-values"); + RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC); If this is now a cc1-only option, this part goes away right?

[PATCH] D60274: [ELF] Implement Dependent Libraries Feature

probinson added a comment. I'm okay with the PS4-specific bits. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60274/new/ https://reviews.llvm.org/D60274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

probinson added a comment. I'm okay with this, but give @aprantl a chance to confirm. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58033/new/ https://reviews.llvm.org/D58033 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

probinson added a comment. I had tried to do this in r11 but some bots complained, so I reverted it and then didn't follow through. Thanks for doing this! When I tried it, I took advantage of existing tracking of line directives at the file level, so there shouldn't be a need to add a Line

[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] D66328: [DebugInfo] Add debug location to dynamic atexit destructor

probinson added inline comments. Comment at: test/CodeGenCXX/debug-info-atexit-stub.cpp:14 + +// CHECK: define internal void @"??__Ff@?1??d@@YAPEAU?$c@UBXZ@YAXXZ"() +// CHECK-SAME: !dbg ![[SUBPROGRAM:[0-9]+]] { Do these Windows-mangled names work on Linux?

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

probinson added a comment. In D66328#1647094 , @aganea wrote: > In D66328#1647062 , @probinson wrote: > > > I don't see a test for the __cxx_global_array_dtor case? > > > It is actually the `arraydestroy.*` loop

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

probinson added a comment. I don't see a test for the __cxx_global_array_dtor case? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66328/new/ https://reviews.llvm.org/D66328 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D64672: [X86] Prevent passing vectors of __int128 as in llvm IR

probinson added a comment. It looks to me like the patch does things the New Way only for Linux and NetBSD, so for PS4 backward compatibility I am okay with it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64672/new/ https://reviews.llvm.org/D64672

[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] 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

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

[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:

[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

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added inline comments. Comment at: clang/include/clang/Driver/Options.td:1955 Flags<[CC1Option]>; +def fdebug_default_version: Joined<["-"], "fdebug-default-version=">, Group; def fdebug_prefix_map_EQ If this is specifically the default DWARF

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added a comment. In D69822#1733243 , @dblaikie wrote: > In D69822#1733226 , @probinson wrote: > > > > Want to decouple setting the DWARF version from enabling/disabling > > > generation of debug info. >

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added a comment. In D69822#1733269 , @dblaikie wrote: > In D69822#1733262 , @probinson wrote: > > > The maze of twisty -g passages gets a new secret door. Oh well. > > > If you find this to be a

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added a comment. > Want to decouple setting the DWARF version from enabling/disabling generation > of debug info. Um, why? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69822/new/ https://reviews.llvm.org/D69822 ___ cfe-commits

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added inline comments. Comment at: clang/include/clang/Driver/Options.td:2002 def gno_codeview_ghash : Flag<["-"], "gno-codeview-ghash">, Flags<[CoreOption]>; + def ginline_line_tables : Flag<["-"], "ginline-line-tables">, Flags<[CoreOption]>;

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

probinson added a comment. Is it tested? Intuitively I would expect DW_OP_pick to be kind of an unusual operator, unlikely to be seen in the wild. Repository: rUNW libunwind CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69893/new/ https://reviews.llvm.org/D69893

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6171 + + if (DwarfVersion == 0) +DwarfVersion = ParseDwarfDefaultVersion(getToolChain(), Args); MaskRay wrote: > lang=cpp > if (DwarfVersion == 0) { > DwarfVersion =

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added inline comments. Comment at: clang/include/clang/Driver/Options.td:1955 Flags<[CC1Option]>; +def fdebug_default_version: Joined<["-"], "fdebug-default-version=">, Group; def fdebug_prefix_map_EQ dblaikie wrote: > probinson wrote: > > If this

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added a comment. The test is in the right place, now it needs to behave more like other driver tests. Sorry if it feels like I'm whaling on you, but the driver is a bit of a peculiar beast with an atypical testing mode. Taming it is harder than it looks. Comment

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added a comment. Just a couple of typos, and I'm happy. I agree with the other reviewers on the last needed test tweaks. Comment at: clang/include/clang/Driver/Options.td:1955 Flags<[CC1Option]>; +def fdebug_default_version: Joined<["-"],

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added a comment. Sigh, one last typo. I'm happy otherwise. Comment at: clang/test/Driver/debug-default-version.c:11 +// environment, we should use codeview. You can enable dwarf, which implicitly +// disables codeview, of you can explicitly ask for both if you don't

[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1151 + || Value > 5 + || Value < 1) +TC.getDriver().Diag(diag::err_drv_invalid_int_value) dblaikie wrote: > probinson wrote: > > Clang doesn't support DWARF v1.

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

probinson added inline comments. Comment at: llvm/test/DebugInfo/X86/debug-info-auto-return.ll:30 +; CHECK: DW_TAG_subprogram [7] * +; CHECK-NEXT: DW_AT_linkage_name [DW_FORM_strx1](indexed (0007) string = "_ZN7myClass7findMaxEv") +; CHECK: DW_AT_type [DW_FORM_ref4]

[PATCH] D71185: [DWARF5] Start emitting DW_AT_dwo_name when -gdwarf-5 is specified.

probinson added a comment. Address the inline comments and LGTM. Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:866 + ? dwarf::DW_AT_dwo_name + : dwarf::DW_AT_GNU_dwo_name;

[PATCH] D71185: [DWARF5] Start emitting DW_AT_dwo_name when -gdwarf-5 is specified.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. Yep LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71185/new/ https://reviews.llvm.org/D71185 ___ cfe-commits mailing list

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

probinson added a comment. > Hmm, maybe this feature/suggestion is broken or at least not exactly awesome > when it comes to auto-returning functions that are eventually void-returning > functions? Now the function definition has no DW_AT_type to override the > unspecified_type in the

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

probinson added a comment. > "DW_TAG_unspecified_type auto" should be emitted for the function > declared/defined as auto returnning. Do you have other test cases in mind, > where above points diverges ?? The declaration would have DW_AT_type point to DW_TAG_unspecified_type, but the

[PATCH] D70537: [clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

probinson added a comment. @dblaikie let me reflect this back to make sure I get it: Template members (methods or variables) would never appear in the *metadata* description of the struct; but metadata descriptions of the instances would refer back to that struct (as the scope for the

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

probinson added a comment. In D70524#1771522 , @shafik wrote: > @probinson I was reading the C++ "auto" return type > issue and I can't come up > with a case where we don't have class descriptions

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

probinson added a comment. In D70524#1772117 , @dblaikie wrote: > In D70524#1771709 , @probinson wrote: > > > In D70524#1771522 , @shafik wrote: > > > > > @probinson I was

[PATCH] D70524: Support DebugInfo generation for auto return type for C++ functions.

probinson added a comment. > Perhaps we should implement this mode under -fno-standalone-debug (or > something more aggressive) since "standalone" is one of the only places I can > think of where having the full class definition would be handy You'd also want it for type units, so they

[PATCH] D71508: [DebugInfo] Duplicate file names in debug info

probinson added a comment. Do we have a similar problem if the filespec has an embedded ./ or ../ in it? I'm thinking some broader canonicalization ought to be done here. $ clang ./dir1/dir2/../dir3/file.c should resolve to dir1/dir3/file.c shouldn't it? Repository: rG LLVM Github Monorepo

[PATCH] D70537: [clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

probinson added a comment. I have to wonder if we're not being too eager to produce the debug info. It seems that the undeduced type problem arises because we're trying to produce debug info before we've really finished instantiating `value` here. But how Clang works pretty much always

[PATCH] D71508: [DebugInfo] Duplicate file names in debug info

probinson added a comment. In D71508#1786122 , @kamleshbhalui wrote: > In D71508#1785767 , @probinson wrote: > > > Do we have a similar problem if the filespec has an embedded ./ or ../ in > > it? > > > problems

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

[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

[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

[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

[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

[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

[PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

probinson added a comment. In D49466#1761156 , @MaskRay wrote: > The ugly path separator pattern `{{(/|)}}` appears in 60+ tests. Can we > teach clang and other tools to > > 1. accept both `/` and `\` input In general they do, AFAIK, although it's

[PATCH] D70537: [clang] CGDebugInfo asserts `!DT.isNull()` when compiling with debug symbols

probinson added a comment. I guess first I'm confused about why the type would be undeduced in the first place, given that it is actually instantiated. And if undeduced is correct, wouldn't we rather emit these with DW_TAG_unspecified_type? Comment at:

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

probinson added a comment. For the case: cat def.c int global_var = 2; def.o should have debug info for the definition of global_var. For the case: cat noref.c extern int global_var; int main() {} I would not expect to see debug info for the declaration of global_var.

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

probinson added a comment. In D70696#1765671 , @dblaikie wrote: > In D70696#1765637 , @probinson wrote: > > > For the case: > > > > cat ref.c > > extern int global_var; > > int main() { return

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

probinson added a comment. In D70696#1765675 , @yonghong-song wrote: > It is just strange that gcc 7.3.1 (did not test, but maybe later gcc versions > as well) emits the debuginfo (encoded in dwarf) > even if the external variable is not used in the

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

probinson added a subscriber: rsmith. probinson added a comment. In D69393#1729772 , @yonghong-song wrote: > During experimenting with linux kernel codes, I found that clang does not > allow address_space attribute for function pointers, specifically,

[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

[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: > >

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

[PATCH] D73261: [dwarf5] Support DebugInfo for constexpr for C++ variables and functions

probinson added a comment. In D73261#1838552 , @djtodoro wrote: > > Is it necessary to use DIFlags? I am willing to do that but generally, it > > is not welcomed because we have a limited number of DIFlags and most of > > them are currently in use. > >

[PATCH] D73261: [dwarf5] Support DebugInfo for constexpr for C++ variables and functions

probinson added a comment. I put in a lot of comments about spelling for the new parameter (constExpr, isConstexpr, isConstExpr) which should be named consistently throughout. Please do not use Constant or any variant, as that tends to mean something else. But, what I would rather see instead

[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] 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

[PATCH] D74324: Tools emit the bug report URL on crash

probinson added a comment. Hi @leonardchan Owen is on UK time so I took a look. I think Owen inadvertently put the new API in a place that is guarded by `#if ENABLE_BACKTRACES` and I'm guessing that your build has that turned off. I've posted D76893 because

[PATCH] D76377: Correctly initialize the DW_AT_comp_dir attribute of Clang module skeleton CUs

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2499 +DIB.createFile(Mod.getModuleName(), TheCU->getDirectory()), +TheCU->getProducer(), false, StringRef(), 0, Mod.getASTFile(), +llvm::DICompileUnit::FullDebug, Signature);

[PATCH] D86965: Do not emit "-tune-cpu generic" for PS4 platform

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. Couple of nits and LGTM. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2078 - // Default to "generic" unless -march is present. + // Default to "generic"

[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/

[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

[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

[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:

[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

[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 = ::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

[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

[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

[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

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

[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] D92606: Clean up debug locations for logical-and/or expressions

probinson created this revision. probinson added reviewers: dblaikie, rnk. probinson added a project: debug-info. probinson requested review of this revision. Herald added a project: clang. Attaches a more appropriate debug location to the PHIs used for the short-circuit evaluations, and makes

[PATCH] D92606: Clean up debug locations for logical-and/or expressions

probinson added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4293 + { +// There is no need to emit line number for unconditional branch. +auto NL = ApplyDebugLocation::CreateEmpty(CGF); rnk wrote: > I see this is consistent with LAnd

[PATCH] D83892: [clang][cli] Port CodeGen option flags to new option parsing system

probinson added a comment. One thing this patch does, is make decisions about default behavior static. Meaning, the option behavior cannot depend on other options; specifically, it can't be based on the triple, which allows target-specific customization. PS4 certainly has cases where our

[PATCH] D83892: [clang][cli] Port CodeGen option flags to new option parsing system

probinson added a comment. Thanks Duncan! I (or someone) will play around with that and see what we need to do. May take a little while, as we're about to freeze a release and then go on break for two weeks, but good to know there's a straightforward path. Repository: rG LLVM Github

[PATCH] D99409: [clang] Speedup line offset mapping computation

probinson added inline comments. Comment at: clang/lib/Basic/SourceManager.cpp:1255 -#ifdef __SSE2__ -#include -#endif +// Check if mutli-byte word x has bytes between m and n, included. This may also +// catch bytes equal to n + 1. Typo: multi-byte. Also,

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

probinson added a comment. In D103125#2781239 , @jhenderson wrote: > In D103125#2780936 , @dblaikie > wrote: > >> Can't say I'm super enthusiastic about this (I assume the build already >> supports prefixes

[PATCH] D103131: support debug info for alias variable

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4957 + auto Loc = getLineNumber(D->getLocation()); + DBuilder.createGlobalVariableExpression( + DContext, Name, StringRef(), Unit, Loc, Ty, I wasn't saying that gcc did the

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2791881 , @dblaikie wrote: > In D103131#2789493 , @probinson > wrote: > >>> Mixed feelings - somewhat in favor of "do the thing that's probably already >>> fairly

[PATCH] D103131: support debug info for alias variable

probinson added a comment. > Mixed feelings - somewhat in favor of "do the thing that's probably already > fairly tested/known to work" (GCC's thing). But open to the idea that that > approach has problems, for sure. "Known to work" for whom? gdb, sure, because gcc/gdb cooperate on many

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2792364 , @dblaikie wrote: > I will say, as the person who implemented DW_TAG_imported_declaration support > in Clang - it's a bit not great/unfortunate (in part because we currently > have to emit them for all

[PATCH] D103125: [Clang][WIP] Allow renaming of "clang"

probinson added a comment. In D103125#2782096 , @echristo wrote: > I'm also not a fan of this change. From a project perspective the binary is > clang and while people may wish to change the name for their own product > teams it seems like that onus

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2780997 , @dblaikie wrote: > Looks like GCC emits aliases as a `DW_TAG_variable` without a location, not > as a `DW_TAG_imported_declaration` and marks it external; this works only because gdb will look up the ELF

[PATCH] D103131: support debug info for alias variable

probinson added a comment. Of course, if it turns out that gdb can't handle the imported_declaration, we might end up having to do this two different ways under the tuning option. I'd *really* prefer not to do that though, and I'd argue it's a gdb bug if it cannot understand

[PATCH] D103131: support debug info for alias variable

probinson added a comment. In D103131#2789493 , @probinson wrote: >> Mixed feelings - somewhat in favor of "do the thing that's probably already >> fairly tested/known to work" (GCC's thing). But open to the idea that that >> approach has problems, for

[PATCH] D97187: [Clang][Sema] Warn when function argument is less aligned than parameter

probinson added a comment. Looks like this caused PR49534 (https://bugs.llvm.org/show_bug.cgi?id=49534) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97187/new/ https://reviews.llvm.org/D97187 ___

[PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

probinson added a comment. > Currently when we have a member function that has an auto return type and the > definition is out of line we generate two DWARF DIE. > One for the declaration and a second one for the definition, the definition > holds the deduced type but does not contain a

[PATCH] D101259: [clang-tidy] Fix cppcoreguidelines-pro-type-vararg false positives with __builtin_ms_va_list

probinson added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp:7 + +// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-vararg %t + TWeaver wrote: > njames93 wrote: > > TWeaver wrote: > > >

[PATCH] D101259: [clang-tidy] Fix cppcoreguidelines-pro-type-vararg false positives with __builtin_ms_va_list

probinson added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp:7 + +// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-vararg %t + probinson wrote: > TWeaver wrote: > > njames93 wrote: > > >

[PATCH] D96354: Avoid conflicts between debug-info and pseudo-probe profiling

probinson created this revision. probinson added reviewers: hoy, wmi. probinson requested review of this revision. Herald added a project: clang. After D93264 , using both -fdebug-info-for-profiling and -fpseudo-probe-for-profiling will cause the compiler to

[PATCH] D96274: [clang][cli] Generate and round-trip Diagnostic options

probinson added a comment. I don't claim to be very familiar with this area but "round-trip" and "test" would make me think those bits should be in a lit test or unittest. As it is, it's not obvious what is functional and what is testing here. Possibly I am misunderstanding something

[PATCH] D96354: Avoid conflicts between debug-info and pseudo-probe profiling

probinson added a comment. > the driver had a redundant pass-through of the option I could've sworn it worked to remove that, but it didn't when I rebased, so that's gone from the final patch (i.e, the explicit pass-through in the driver is still there). It's a functionally separate topic

<    1   2   3   4   5   6   >