[PATCH] D46767: Force the PS4 clang ABI version to 6, and add a warning if this is attempted to be overridden

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D46767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/

[PATCH] D46767: Force the PS4 clang ABI version to 6, and add a warning if this is attempted to be overridden

probinson added a comment. In https://reviews.llvm.org/D46767#1096746, @rsmith wrote: > Everything old is new again. If only that were true about my brain. :-P > This was discussed when `-fclang-abi-compat` was introduced; see > https://reviews.llvm.org/D36501 for the argument why this patch

[PATCH] D46982: Do not enable RTTI with -fexceptions, for PS4

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. So on PS4, `-fsanitize=vptr` without an explicit `-frtti` will warn and disable the sanitizer, rather than implicitly enabling RTTI. As well as exceptions no longer implicitly enabling

[PATCH] D46439: [Sema] Fix incorrect packed aligned structure layout

probinson added a comment. This wouldn't be another layout/ABI breakage, would it? Repository: rC Clang https://reviews.llvm.org/D46439 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D47161: update

probinson added a comment. I take it this was accidental? If there are weaknesses in our documentation for how to use Phabricator, please let us know. I know it is not always straightforward (I had a number of issues when I tried to start using it). Repository: rC Clang https://reviews.ll

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

probinson added a reviewer: rnk. probinson added a subscriber: rnk. probinson added a comment. + @rnk who did the initial checksum stuff for CodeView. I am unclear how the notion of checksums should interact with preprocessed files. Nit: while many tests have dates in the filenames, we no longe

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

probinson added a comment. After thinking about this a bit... The # directive will cause the filename to be identified as the source for the subsequent lines. That means it will show up as the original source file in the line table. However, the compiler doesn't have the actual source file of

[PATCH] D47097: [DebugInfo] Preserve scope in auto generated StoreInst

probinson added a comment. Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me. The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setu

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

probinson added a comment. I see that the per-file info does track the presence of `# [line] N` but it's not immediately obvious how to query "has any file seen one" which is really what I'd want to know. The file information has various levels of indirection... Repository: rC Clang https

[PATCH] D47375: [Driver] Add flag "--dependent-lib=..." when enabling asan or ubsan on PS4.

probinson added a comment. LGTM with the indicated test tweak, but best if @filcab also takes a look. Comment at: lib/Driver/ToolChains/PS4CPU.cpp:87 +CmdArgs.push_back("--dependent-lib=libSceDbgAddressSanitizer_stub_weak.a"); + } +} Don't bother with brac

[PATCH] D47260: Testcase for dwarf 5 crash/assert when calculating a checksum for an expansion

probinson added a comment. @trixirt do you mind if I commandeer this review? I think I have a patch. Repository: rC Clang https://reviews.llvm.org/D47260 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D47291: Proposal to make rtti errors more generic.

probinson added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6729 def err_no_dynamic_cast_with_fno_rtti : Error< - "cannot use dynamic_cast with -fno-rtti">; + "use of dynamic_cast requires enabling RTTI">; filcab wrote: > I'd pref

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

probinson updated this revision to Diff 148663. probinson added a comment. Upload patch to suppress checksums when we see a preprocessed file. https://reviews.llvm.org/D47260 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/CGDebugInfo.h clang/test/CodeGen/md5-checksum-crash.c

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

probinson marked an inline comment as done. probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:378 return None; + if (Entry.getFile().hasLineDirectives()) { +EmitFileChecksums = false; aprantl wrote: > Can you add a comment e

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

This revision was automatically updated to reflect the committed changes. probinson marked an inline comment as done. Closed by commit rC11: [DebugInfo] Don't bother with MD5 checksums of preprocessed files. (authored by probinson, committed by ). Changed prior to commit: https://reviews.ll

[PATCH] D47260: [DebugInfo] Skip MD5 checksums of preprocessed files

This revision was automatically updated to reflect the committed changes. probinson marked an inline comment as done. Closed by commit rL11: [DebugInfo] Don't bother with MD5 checksums of preprocessed files. (authored by probinson, committed by ). Herald added a subscriber: llvm-commits. Chan

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

probinson updated this revision to Diff 116711. probinson added a comment. Add a command-line flag to control emitting the template parameter children. Default to on for SCE debugger tuning. I am not super excited about my choice of option name or the help text; alternate suggestions welcome. I

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

probinson added a comment. In https://reviews.llvm.org/D14358#881666, @aprantl wrote: > Does this have to be exposed through the driver or could this be a cc1 option > only? My thinking was to make it easier for LLDB to play with it and decide whether DWARF conformance on this point is a good

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

probinson added a comment. In https://reviews.llvm.org/D14358#882445, @dblaikie wrote: > > I would prefer to eliminate the `` from the instance name as well, > > because our debugger reconstructs a name more to its liking from the > > parameter children. However, IIUC the name with params is u

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

probinson updated this revision to Diff 116862. probinson added a comment. command-line option is cc1 not driver internal flag moved from LangOpts to CodeGenOpts and renamed simplified test https://reviews.llvm.org/D14358 Files: include/clang/Driver/CC1Options.td include/clang/Frontend/Code

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

probinson added a reviewer: rnk. probinson added a comment. +rnk for the CodeView question. Comment at: include/clang/Frontend/CodeGenOptions.def:222 ///< of inline stack frames without .dwo files. +CODEGENOPT(DebugFwdTemplateParams, 1, 0)

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

probinson added a comment. I think I will go ahead and commit this; it doesn't change the status quo for CodeView and if something is warranted we can do that in a follow-up. https://reviews.llvm.org/D14358 ___ cfe-commits mailing list cfe-commits@

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL31: [DWARF] Allow forward declarations of a class template instantiation (authored by probinson). Changed prior to commit: https://reviews.llvm.org/D14358?vs=116862&id=117030#toc Repository: rL L

[PATCH] D35715: Preserve typedef names in debug info for template type parameters

probinson abandoned this revision. probinson added a comment. Abandoning. This change is irrelevant to the SCE debugger, and while I believe it could be made more complete and better reflect the original source (which is what the DWARF spec says names should be), I do not have time to pursue it

[PATCH] D39069: CodeGen: Fix missing debug loc due to alloca

probinson added a comment. Anytime the code between saveIP() and restoreIP() could set the current debug location, it needs to be saved/restored along with the insertion point. I have to say the problem is not obvious to me here, so maybe saveIP/restoreIP should be changed (or eliminated in fa

[PATCH] D39239: [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

probinson added a comment. Have you tried this change against the GDB and LLDB test suites? If they have failures then we should think about whether those tests are over-specific or whether we should put this under a tuning option. https://reviews.llvm.org/D39239 __

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

probinson added subscribers: cfe-commits, probinson. probinson added a comment. +cfe-commits https://reviews.llvm.org/D51340 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

probinson added a comment. @rsmith any further thoughts? We would really like to get this in before the LLVM 7.0 branch, currently scheduled for 1 August. In https://reviews.llvm.org/D46190#1168027, @CarlosAlbertoEnciso wrote: > @probinson: I tried `clang-format-diff` and the main format issue

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

probinson added a comment. Is this because type units depend on COMDAT support? I had a vague idea that COFF also supports COMDAT. https://reviews.llvm.org/D49594 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bi

[PATCH] D49594: [DebugInfo] Emit diagnostics when enabling -fdebug-types-section on non-linux target.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. If the plan is for this to be relatively temporary then LGTM. https://reviews.llvm.org/D49594 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

probinson added a reviewer: probinson. probinson added a comment. Needs a test. Repository: rC Clang https://reviews.llvm.org/D49652 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

probinson added a comment. Also, please document the option in clang/docs/UsersManual.rst. It should have been added when the option was first added, but certainly with right-to-left behavior it needs a mention. Nearly all options work left-to-right, so even though it's the same as gcc, not e

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

probinson added a comment. Re. SmallVector versus std::vector, they are functionally similar but have different memory-allocation behaviors. SmallVector includes a vector of N elements (where N is the template parameter) so does no dynamic allocation until you have more than N elements; but it

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

probinson added a comment. In https://reviews.llvm.org/D49652#1172352, @alxu wrote: > I was just going to run llvm-dwarfdump. Well, that's one of those quirks I mentioned. This is a Clang change, and Clang tests should exercise as little of LLVM as possible. That means you don't generate as

[PATCH] D49652: Apply -fdebug-prefix-map in reverse of command line order

probinson added a comment. In https://reviews.llvm.org/D49652#1172498, @alxu wrote: > my general theory is that it's better to have tests that test everything at > once if possible, but I guess it's unlikely enough that the debug info path > will be correct in the IR and then not correct in the

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

probinson accepted this revision. probinson added a comment. Although I am far from expert in how Clang manages declarations, AFAICT this does what @rsmith requested, so LGTM. https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits

[PATCH] D49953: [compiler-rt] Add a routine to specify the mode used when creating profile dirs.

probinson added a comment. Is it possible/reasonable for the test to call __llvm_profile_recursive_mkdir and then verify the mode of the created directory? The test would have to be flagged as unsupported on Windows but that seems fine. https://reviews.llvm.org/D49953 ___

[PATCH] D49953: [compiler-rt] Add a routine to specify the mode used when creating profile dirs.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. Using `%t` with suffixes instead of `%T` is the preferred idiom, glad you worked that out. The more elaborate test depends on a set of Posix-y calls, but that's fine. If there are more

[PATCH] D43189: [DebugInfo] Avoid name conflict of generated VLA expression variable.

probinson added a comment. Please make sure to cite the PR in the commit message. https://reviews.llvm.org/D43189 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42766: [DebugInfo] Support DWARFv5 source code embedding extension

probinson added a comment. Looks generally straightforward. I'd give other people a chance to chime in but I have only minor comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:411 // If the location is not valid then use main input file. -return DBuilder.createFile(re

[PATCH] D43700: Emit proper CodeView even when not using the cl driver.

probinson added a comment. Being a cross-compiler I think it's generally a good thing to have more combinations be less broken. Note that PS4's compiler is hosted on Windows and uses the gcc-style driver; it's convenient sometimes to be able to target Windows without having to learn a new drive

[PATCH] D39239: [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

probinson added a project: debug-info. probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. With the GDB test results and LLDB able to handle it, this LGTM. Carlos, do you have commit access? https://reviews.llvm.org/D39239

[PATCH] D39239: [AST] Incorrectly qualified unscoped enumeration as template actual parameter.

This revision was automatically updated to reflect the committed changes. Closed by commit rC321312: [AST] Incorrectly qualified unscoped enumeration as template actual parameter. (authored by probinson, committed by ). Changed prior to commit: https://reviews.llvm.org/D39239?vs=120066&id=12793

[PATCH] D41421: Eliminate a magic number. NFC.

This revision was automatically updated to reflect the committed changes. Closed by commit rC321757: Calculate size of buffer instead of using a magic value. (authored by probinson, committed by ). Repository: rC Clang https://reviews.llvm.org/D41421 Files: lib/Frontend/PrintPreprocessedOut

[PATCH] D47291: Proposal to make rtti errors more generic.

probinson added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6729 def err_no_dynamic_cast_with_fno_rtti : Error< - "cannot use dynamic_cast with -fno-rtti">; + "use of dynamic_cast requires enabling RTTI">; wristow wrote: > Sunil_S

[PATCH] D47291: Proposal to make rtti errors more generic.

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

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

probinson added a comment. @rsmith anything else needed here? https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

probinson added a comment. @dblaikie is @rsmith back from the standards meeting yet? I hate to be a pest but this is blocking other work Carlos has in progress. https://reviews.llvm.org/D46190 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

probinson added a comment. Style comments. The two new Sema methods (for namespaces and using references) are C++ specific, so SemaDeclCXX.cpp would seem like a more appropriate home for them. Comment at: lib/Sema/Sema.cpp:1879 +void Sema::MarkNamespaceAliasReferenced(NamedDec

[PATCH] D47720: [DebugInfo] Inline for without DebugLocation

probinson added a comment. In https://reviews.llvm.org/D47720#1136585, @gramanas wrote: > ping! should I commit this? @vsk accepted it, so yes go ahead. Repository: rC Clang https://reviews.llvm.org/D47720 ___ cfe-commits mailing list cfe-comm

[PATCH] D57162: [DEBUG_INFO][NVPTX] Generate correct data about variable address class.

probinson added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:4235 CGM.getContext().getTargetAddressSpace(D->getType()); +if (CGM.getLangOpts().CUDA && CGM.getLangOpts().CUDAIsDevice) { + if (D->hasAttr()) Can a variable have one of t

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

probinson added a comment. In https://reviews.llvm.org/D52296#1282458, @dblaikie wrote: > I guess in that case your distributed build system would have a constraint > that it always ships all the object files back to the primary machine (where > you'd be running the debugger)? (perhaps it just

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

probinson added a comment. In https://reviews.llvm.org/D52296#1282589, @dblaikie wrote: > In https://reviews.llvm.org/D52296#1282587, @probinson wrote: > > > Any distributed build has to make this work, so the paths in the line table > > are usable. Not clear what you're thinking might be the p

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

probinson added a comment. In https://reviews.llvm.org/D52296#1283691, @grimar wrote: > Nice :) > So seems the last unresolved question left is the naming of the new option? > If we do not want to go with `-gsingle-file-split-dwarf` then what it should > be? > > What about `-fdebug-fission` a

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

probinson added subscribers: gbedwell, probinson. probinson added a comment. +gbedwell Just to throw the idea out there, why not abandon debuginfo-tests entirely and try using Dexter for this. Dexter's design center is debug-info quality, not correctness, but it already knows how to drive sever

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

probinson added a comment. In https://reviews.llvm.org/D54187#1290294, @zturner wrote: > In https://reviews.llvm.org/D54187#1290282, @probinson wrote: > > > +gbedwell > > > > Just to throw the idea out there, why not abandon debuginfo-tests entirely > > and try using Dexter for this. Dexter's de

[PATCH] D54187: Add debuginfo-tests that use cdb on Windows

probinson added a comment. In https://reviews.llvm.org/D54187#1290340, @gbedwell wrote: > (Dexter) will step through every line of the program it can, collecting info > about each step until it reaches the program exit. It won't currently > produce a pass/fail, but rather a score. That is, if

[PATCH] D52296: [Clang] - Add -fdwarf-fission=split,single option.

probinson added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:5889 const llvm::Triple &T = getToolChain().getTriple(); - if (Args.hasArg(options::OPT_gsplit_dwarf) && + if ((getDebugFissionKind(D, Args) == DwarfFissionKind::Split) && (T.isOSLinux() || T

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

probinson created this revision. probinson added reviewers: dblaikie, aprantl. probinson added a project: debug-info. Herald added a subscriber: eraman. See https://reviews.llvm.org/D54755 Repository: rC Clang https://reviews.llvm.org/D54756 Files: clang/test/CodeGen/debug-info-scope-file.

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

probinson added a comment. Ping. @aprantl approved the dependent patch D54755 . Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54756/new/ https://reviews.llvm.org/D54756 ___ cfe-commi

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

probinson added a comment. In D54756#1311679 , @aprantl wrote: > Everybody with out-of-tree tests will be excited ;-) Yeah... fortunately we still accept the old syntax on input, so it's only people with checks on DISubprogram *output* that will need to

[PATCH] D54756: [DebugInfo] NFC Clang test changes for DISubprogram flags

This revision was automatically updated to reflect the committed changes. Closed by commit rL347807: [DebugInfo] NFC Clang test changes for: IR/Bitcode changes for DISubprogram… (authored by probinson, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://rev

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

probinson added a comment. > When building the FileCheck binary with debug info, this patch makes the > build artifacts ~1kb smaller. Nice! Comment at: lib/IR/DiagnosticInfo.cpp:39 #include "llvm/Support/ScopedPrinter.h" +#include "llvm/Support/raw_ostream.h" #include

[PATCH] D55085: Avoid emitting redundant or unusable directories in DIFile metadata entries

probinson added inline comments. Comment at: lib/IR/DiagnosticInfo.cpp:39 #include "llvm/Support/ScopedPrinter.h" +#include "llvm/Support/raw_ostream.h" #include aprantl wrote: > probinson wrote: > > Do we use a case-sensitive sort of include files? I thought

[PATCH] D55137: Honor -fdebug-prefix-map when creating function names for the debug info.

probinson added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:238 + // Apply -fdebug-prefix-map. + PP.RemapFilePaths = true; + PP.remapPath = [this](StringRef Path) { return remapDIPath(Path); }; Unconditionally? CHANGES SINCE LAST ACTION https:

[PATCH] D55137: Honor -fdebug-prefix-map when creating function names for the debug info.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM aside from a grump about the test, which is totally up to you. Comment at: test/CodeGenCXX/debug-prefix-map-lambda.cpp:7 + // CHECK: !DISubprogram(name: "b<(lambd

[PATCH] D15881: [DWARF] Omitting the explicit import of an anonymous namespace is a debugger-tuning decision, not a target decision.

probinson abandoned this revision. probinson added a comment. Herald added a subscriber: JDevlieghere. Abandoning dead patch. This wound up being done a different way. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D15881/new/ https://reviews.llvm.org/D15881 __

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

probinson added a comment. I'd preserve the trivial test cases and show that the NonTrivial flag is *not* present. I can suggest ways to achieve this if you like (note there is no CHECK-DAG-NOT combined directive). Comment at: test/CodeGenCXX/debug-info-composite-triviality.

[PATCH] D59815: [Driver] Enable -fsanitize-address-globals-dead-stripping by default on PS4.

probinson added a comment. This is fine for PS4, I'm just curious whether anyone knows if there's a predicate that means something like "target does not use GNU tools" so we don't have to itemize Fuschia and PS4 this way. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D59008: [AMDGPU] Switch default dwarf version to 5

probinson added a comment. In D59008#1442515 , @dblaikie wrote: > In D59008#1442256 , @t-tye wrote: > > > In D59008#1442014 , @dblaikie > > wrote: > > > > > In D59008#144190

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

probinson added a comment. As a rule I would prefer flags with positive names, as it's slightly easier to read `!isTrivial` than `!isNonTrivial`. And trivially shorter. :-) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59347/new/ https://reviews.llvm.org/D59347

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

probinson added a comment. In D59347#1444819 , @dblaikie wrote: > In D59347#1443051 , @dblaikie wrote: > > > @asmith: Where's the LLVM-side change/review that goes with this, btw? > > > > In D59347#1442970

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

probinson added a comment. Commentary questions. Comment at: lib/Driver/ToolChains/Clang.cpp:3166 + // If you say "-gsplit-dwarf -gline-tables-only", -gsplit-dwarf loses. + // But -gsplit-dwarf is not a g_Group option, hence we have to check the + // order explic

[PATCH] D59923: [Driver] Simplify -g level computation and its interaction with -gsplit-dwarf

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM. I wish it had occurred to me to pass both OPT_g_Group and OPT_gsplit_dwarf to the same getLastArgs call in the first place. Repository: rC Clang CHANGES SINCE LAST ACTION ht

[PATCH] D59815: [Driver] Enable -fsanitize-address-globals-dead-stripping by default on PS4.

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/D59815/new/ https://reviews.llvm.org/D59815 ___ cfe-comm

[PATCH] D59347: [DebugInfo] Combine Trivial and NonTrivial flags

probinson added inline comments. Comment at: test/CodeGenCXX/debug-info-composite-triviality.cpp:88 - -// CHECK-DAG: !DICompositeType({{.*}}, name: "NonTrivialE",{{.*}}flags: {{.*}}DIFlagNonTrivial -struct NonTrivialE : Trivial, NonTrivial { Hui wrote: > probins

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

probinson added a comment. I guess I'm not clear what your final goal is for the option. Keep it, even though GCC doesn't have one like it? Eliminate it? Please clearly state what you intend to have in the end, and what you might have in the short term in case that is different. =

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

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

[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 https://lists.llvm.org/cgi-bin/

[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 https://l

[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] 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] D61187: [clangd] Move clangd tests to clangd directory. check-clangd is no longer part of check-clang-tools.

probinson added a comment. FYI, we had to disable clangd entirely after this patch, getting a weird problem with lit that we haven't figured out yet. In case anybody else has seen something like this, and knows what's going on, please let us know. Running all regression tests llvm-lit.p

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

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

probinson added a comment. Do we generate the .dwo file directly these days? If not, I can imagine wanting to avoid the overhead of the objcopy hack; as long as the linker is smart enough not to bother with the .debug_*.dwo sections this seems like a build-time win. https://reviews.llvm.org/

[PATCH] D57162: [DEBUG_INFO][NVPTX] Generate correct data about variable address class.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. Herald added a project: clang. LGTM. I'll trust you on the actual address-class values. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57162/new/ https://

[PATCH] D57896: Variable names rule

probinson added a comment. Herald added a project: LLVM. In D57896#1389067 , @lebedev.ri wrote: > 2. It might be best to give this more visibility, by submitting a mail to > llvm-dev, with a **noticeable** subject, like "RFC: changing variable naming > r

[PATCH] D58061: Fix a few tests that were missing ':' on CHECK lines and weren't testing anything.

probinson added a comment. There has been some progress recently on better FileCheck diagnosis of likely test-writing issues, although to date it mostly requires the human to ask "what is going on here?" explicitly. I can see adding a check to detect the missing-colon-on-otherwise-valid-direct

[PATCH] D58035: [clang/DIVar] Emit flag for params that have unchanged values

probinson added inline comments. Comment at: include/clang/AST/Decl.h:1482 + bool getIsArgumentModified() const { +return IsArgumentModified; There should be a comment here. The style in this class appears to omit the 'get' word from the name of the gette

[PATCH] D57896: Variable names rule

probinson added a comment. In D57896#1406353 , @MyDeveloperDay wrote: > I can't argue with anything you say.. but I guess to reinforce your point > introducing what is effectively a 3rd style would likely cause even more > jarring... Zach isn't introd

[PATCH] D57896: Variable names rule

probinson added a comment. In D57896#1406412 , @MyDeveloperDay wrote: > In D57896#1406407 , @zturner wrote: > > > If I read the post correctly, it was actually agreeing with me (because it > > said "to reinforce y

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

probinson added a comment. I would prefer a diagnostic if PS4 && >3.2. Whether you map "latest" to 3.2 or diagnose that also, up to you, I'm okay either way. Repository: rL LLVM https://reviews.llvm.org/D36501 ___ cfe-commits mailing list cfe-c

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

probinson added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:2934 +ABICompatArg->render(Args, CmdArgs); + // Add runtime flag for PS4 when PGO or Coverage are enabled. ``` else if (getToolChain().getTriple().isPS4()) CmdArgs.push_back("-f

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

probinson added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:2934 +ABICompatArg->render(Args, CmdArgs); + // Add runtime flag for PS4 when PGO or Coverage are enabled. rsmith wrote: > probinson wrote: > > ``` > > else if (getToolChain().get

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

probinson added inline comments. Comment at: lib/CodeGen/CGVTables.cpp:157 + if (DebugInfo) +DebugInfo->replaceTemporaryNodes(); + aprantl wrote: > Have you looked at what it would take to only finalize the nodes reachable > via the function? > Otherwise —

[PATCH] D36501: Add flag to request Clang is ABI-compatible with older versions of itself

probinson added a comment. Sorry it took so long to wrap my head around this, but it has been about 20 years since I've had to accommodate an intentional ABI breakage, and that's actually what you want to do. Remembering that case, I have a model for this one, and I understand what you are doi

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

probinson added a comment. Trying to understand the broader context here, I looked back through the list of revisions mentioned in PR33930 to see if that helped. When called on a method, CodeGenModule::EmitGlobalDefinition() calls CodeGenModule::EmitGlobalFunctionDefinition(), which in turn cal

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

probinson added a comment. In https://reviews.llvm.org/D37038#854722, @probinson wrote: > finalizeSubprogram() retrieves the variables from the subprogram and handles > them; what is it missing? For temp-md-nodes2.cpp, the assertion in mapTopLevelUniquedNode() trips on a DICompositeType for C

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

probinson added a comment. Dumping the whole module, the temporary DICompositeType node for CBdVfsImpl is used both as the scope node for the DISubprogram, and also as the baseType of a DIDerivedType which is a pointer type in the type list for the subprogram. Given that the DICompositeType is

  1   2   3   4   5   6   >