[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

[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

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

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

[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

[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

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

[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

[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

[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

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

[PATCH] D42248: [LangOpts] Add a LangOpt to represent "#pragma region" support.

probinson added a comment. Well, my understanding is that the pragma is a complete no-op even for MSVC, and is used only as a marker for editors such as Visual Studio's. So, unconditionally ignoring it would seem to be fine. https://reviews.llvm.org/D42248

[PATCH] D42370: Issue local statics in correct DWARF lexical scope

probinson added subscribers: cfe-commits, probinson. probinson added a comment. +cfe-commits See also the LLVM change in https://reviews.llvm.org/D42369 and a debuginfo-tests test in https://reviews.llvm.org/D42371. https://reviews.llvm.org/D42370

[PATCH] D42011: [DWARFv5] Enable MD5 checksums

This revision was automatically updated to reflect the committed changes. Closed by commit rC322413: [DWARFv5] Have -gdwarf-5 generate MD5 checksums (authored by probinson, committed by ). Changed prior to commit: https://reviews.llvm.org/D42011?vs=129703=129708#toc Repository: rC Clang

[PATCH] D42011: [DWARFv5] Enable MD5 checksums

This revision was automatically updated to reflect the committed changes. Closed by commit rL322413: [DWARFv5] Have -gdwarf-5 generate MD5 checksums (authored by probinson, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D42011: [DWARFv5] Enable MD5 checksums

probinson created this revision. probinson added reviewers: dblaikie, aprantl. probinson added a project: debug-info. Herald added subscribers: cfe-commits, JDevlieghere. Under `-gdwarf-5` generate MD5 checksums of source files to emit to the DWARF v5 line table. This consumes 16 bytes per

[PATCH] D42581: [NVPTX] Emit debug info in DWARF-2 by default for Cuda devices.

probinson added a comment. If you want to force DWARF 2, probably clamping the version in LLVM would be simpler? Although most of the debug-info tests are architecture-specific and wouldn't run for an NVPTX target anyway. Repository: rC Clang https://reviews.llvm.org/D42581

[PATCH] D42351: Emit DWARF "constructor" calling convention for every supported Clang CC

probinson added a comment. +1 for testcase. Repository: rC Clang https://reviews.llvm.org/D42351 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42758: Support `#pragma comment(lib, "name")` in the frontend for ELF

probinson added a comment. In https://reviews.llvm.org/D42758#993936, @ruiu wrote: > > I also wonder which is better `#pragma comment(lib, "m")` or `#pragma > > comment(lib, "m")`. > > Sorry, I meant `#pragma comment(lib, "m")` or `#pragma comment("lib", "m")`. I can't swear to it but I don't

[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

[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

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

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

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

[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

[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

[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

[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

[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

[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

[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

[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

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

[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=154811#toc

[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] D42371: Issue local statics in correct DWARF lexical scope

probinson added subscribers: cfe-commits, probinson. probinson added a comment. +cfe-commits See also the LLVM change in https://reviews.llvm.org/D42369 and Clang change in https://reviews.llvm.org/D42370. https://reviews.llvm.org/D42371 ___

[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

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

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

[PATCH] D44788: Add an option to support debug fission on implicit ThinLTO.

probinson added a comment. I don't think requiring a new option is a great user interface. In existing use cases the location of the .dwo file matches the location of the output file. Why is this one different? Repository: rC Clang https://reviews.llvm.org/D44788

[PATCH] D44788: Add an option to support debug fission on implicit ThinLTO.

probinson added a comment. In https://reviews.llvm.org/D44788#1046093, @dblaikie wrote: > In implicit ThinLTO, the object files are only temporary. > > Sort of similar to using -gsplit-dwarf when compiling straight to an > executable (without using -c): "clang++ x.cpp y.cpp -o a.out" - where >

[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

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

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

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

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

[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

[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

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

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

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

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

[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

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

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

[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: Dont emit DW_AT_enum_class unless its 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] 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

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

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

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

[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

[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

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

[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

[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

[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

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

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

[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

[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

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

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

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

[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

[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

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

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

[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

[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

[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

[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

<    1   2   3   4   5   6   >