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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5ea2d4fa4811: Avoid conflicts between debug-info and pseudo-probe profiling (authored by probinson). Changed prior to commit:

[PATCH] D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions.

probinson added a comment. In D99250#2651394 , @jsji wrote: > The list is growing, but sure, we will post a thread in llvm-dev about what > we met so far. > Two big one would be that DBX not supporting string section(DW_FORM_strp) and > column-info

[PATCH] D98514: [RGT] Fix ASTMatchersTest so all assertions are executed

probinson abandoned this revision. probinson added a comment. Problem has gone away, test assertions are now executed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98514/new/ https://reviews.llvm.org/D98514

[PATCH] D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions.

probinson added a comment. In D99250#2649507 , @Esme wrote: > Thx! @aprantl The motivation of the patch came from the crash of tag name > mismatching when using DBX under AIX. And modifying the debugger doesn't seem > to make sense? A consumer should

[PATCH] D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions.

probinson added a comment. In D99250#2651953 , @probinson wrote: > In D99250#2651204 , @dblaikie wrote: > >> Does anyone else have a DWARFv3 consumer they care about? (@aprantl and >> @probinson) >> >> Does anyone

[PATCH] D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions.

probinson added a comment. I think there are degrees of compatibility with regard to DWARF, which is inherently supposed to be extensible if the extension can be safely ignored by a consumer. This is the "permissive" rule in the standard. - A new FORM must *never* be used, because that makes

[PATCH] D99250: [DebugInfo] Fix the mismatching of C++ language tags and Dwarf versions.

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:572 LangTag = llvm::dwarf::DW_LANG_C_plus_plus_14; -else if (LO.CPlusPlus11) +else if (LO.CPlusPlus11 && CGM.getCodeGenOpts().DwarfVersion >= 5) LangTag =

[PATCH] D99160: [X86][FastISEL] Support DW_TAG_call_site_parameter with FastISEL

probinson added a comment. FastISel is normally used only at -O0, I wouldn't expect any parameters to be optimized out at -O0. The test is running llc with default optimization, which is -O2, and forcing fast-isel; this is not a usual combination and I wouldn't expect us to spend any effort

[PATCH] D99238: [DebugInfo] Enable the call site parameter feature by default

probinson added a reviewer: dblaikie. probinson added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1648 - if (Opts.OptimizationLevel > 0 && Opts.hasReducedDebugInfo() && + if (Opts.hasReducedDebugInfo() &&

[PATCH] D99199: Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

probinson added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99199/new/ https://reviews.llvm.org/D99199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D99199: Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

probinson marked 2 inline comments as done. probinson added a comment. Addressed comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99199/new/ https://reviews.llvm.org/D99199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D99199: Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

probinson updated this revision to Diff 333884. probinson added a comment. Remove ifdefs, tweak descriptions/comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99199/new/ https://reviews.llvm.org/D99199 Files: clang/lib/Driver/ToolChains/Clang.cpp

[PATCH] D98873: Document -fcrash-diagnostics-dir

This revision was automatically updated to reflect the committed changes. Closed by commit rGe150be612bf7: Document -fcrash-diagnostics-dir (authored by probinson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98873/new/

[PATCH] D99199: Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

probinson created this revision. probinson added a reviewer: rnk. Herald added subscribers: dexonsmith, hiraditya. probinson requested review of this revision. Herald added projects: clang, LLVM. There's no automated test for this, as I don't see any existing tests for creating minidumps. I

[PATCH] D99199: Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG04b3c8c52c54: Pass -fcrash-diagnostics-dir along to LLVM (authored by probinson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D98514: [RGT] Fix ASTMatchersTest so all assertions are executed

probinson created this revision. probinson added a reviewer: Prazek. probinson requested review of this revision. Herald added a project: clang. Some 'if' conditions turn out always to be false, so change test expectations to match. Found by the Rotten Green Tests project. Repository: rG

[PATCH] D98873: Document -fcrash-diagnostics-dir

probinson created this revision. probinson added a reviewer: hans. Herald added subscribers: jansvoboda11, dang. probinson requested review of this revision. Herald added a project: clang. This was added in LLVM 7.0 but without help text or other docs. Repository: rG LLVM Github Monorepo

[PATCH] D98514: [RGT] Fix ASTMatchersTest so all assertions are executed

probinson added a reviewer: gribozavr. probinson added a comment. +gribozavr who has been in this file most recently Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98514/new/ https://reviews.llvm.org/D98514

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

probinson added a comment. > GNU ld has a rule "start_/stop_ references from a live input section retain > the associated C identifier name sections", which LLD may change in the future (D96914 ). The phrasing "may change" implies LLD could eliminate the rule;

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

probinson added a comment. In D97446#2587996 , @MaskRay wrote: > In D97446#2587806 , @probinson wrote: > >>> GNU ld has a rule "start_/stop_ references from a live input section retain >>> the associated C

[PATCH] D96838: Add GNU attribute 'retain'

probinson added a comment. Aha; attribute `used` *by itself* is not sufficient to preserve sections in the output. But the `__start_/__stop_` symbols implicitly create a reference to each of the named sections, and that implicit reference can preserve them in the output (assuming gc roots

[PATCH] D97566: [WIP][RGT] Rotten Green Test checking for LLVM and Clang unittests

probinson created this revision. probinson added reviewers: MaskRay, dblaikie. Herald added a subscriber: mgrang. probinson requested review of this revision. Herald added projects: clang, LLVM. Herald added a subscriber: cfe-commits. This is an enhancement of the 'googletest' support code, to

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

probinson added a comment. In D97411#2598625 , @akhuang wrote: > I started looking into some diffs of debug info in libc++ tests, but it's > pretty hard to tell what's different - as far as I can see, there are just a > bunch of `__hash_value_type`s and

[PATCH] D96838: Add GNU attribute 'retain'

probinson added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:98 +This support is available in GNU ``ld`` and ``gold`` as of binutils 2.36, as +well as in ``ld.lld`` 13. + }]; As a user, this seems like a complicated thing to figure out.

[PATCH] D96838: Add GNU attribute 'retain'

probinson added a comment. > For ELF, `used` does not retain the entity, regardless of this patch. That statement does not match my experience. Note that there are two separate variables documented in the LangRef: `llvm.used` (which corresponds to source attribute `used` and is documented as

[PATCH] D100049: Remove .gitignore entries not relevant in the monorepo.

This revision was automatically updated to reflect the committed changes. Closed by commit rG676a9ab5e406: Remove .gitignore entries not relevant in the monorepo. (authored by probinson). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo

[PATCH] D100298: [Clang][Coroutine][DebugInfo] Follow-up: reduce a tests ordering requirements

probinson added a comment. I am not 100% convinced this has the desired effect. I believe what it does is: - Search for the first occurrence of each of "val" "moParam" and "mcParam" - After the last of the above occurrences, ensure there are no other occurrences of "val" "moParam" or "mcParam"

[PATCH] D100826: [Debug-Info][NFC] add -gstrict-dwarf support in backend

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. You have a clang-format warning, aside from that LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100826/new/

[PATCH] D100809: [Debug-Info] implement -gstrict-dwarf

probinson added a comment. Nit on the description, this patch supports -gstrict-dwarf in the frontend. Comment at: clang/docs/ClangCommandLineReference.rst:3549-3550 +Use DWARF extensions in later DWARF versions. + .. option:: -gz=, -gz (equivalent to -gz=zlib)

[PATCH] D100630: [Debug-Info][DBX] DW_TAG_rvalue_reference_type should not be generated when dwarf version is smaller than 4

probinson added a comment. If DBX is going to be really pedantic about not recognizing tags or attributes that don't align with the DWARF version, maybe we're better off with really supporting `-gstrict-dwarf` and just have DBX tuning imply that. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D100298: [Clang][Coroutine][DebugInfo] Follow-up: reduce a tests ordering requirements

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. One optional nit and LGTM, assuming the DILocalVariable ordering is consistent with what we see downstream. Comment at: clang/test/CodeGenCoroutines/coro-dwarf.cpp:1

[PATCH] D74436: Change clang option -ffp-model=precise to select ffp-contract=on

probinson added a comment. I see that the UserManual has been updated, but this behavior change should probably also be mentioned in the Release Notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74436/new/ https://reviews.llvm.org/D74436

[PATCH] D111352: [clang] Fix absolute file paths with -fdebug-prefix-map

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. I suspect this is not the only way for directories and filenames to become confused, but it certainly helps. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D110129: [DebugInfo] Support typedef with btf_tag attributes

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. In D110129#3013946 , @yonghong-song wrote: > - The only thing left is for llvm/test/DebugInfo/X86/attr-btf_tag-typedef.ll > for which I didn't

[PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. Seems like a good simplification. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110455/new/ https://reviews.llvm.org/D110455

[PATCH] D110129: [DebugInfo] Support typedef with btf_tag attributes

probinson added inline comments. Comment at: clang/test/CodeGen/attr-btf_tag-typedef.c:2 +// REQUIRES: x86-registered-target +// RUN: %clang -target x86_64 -g -S -emit-llvm -o - %s | FileCheck %s + Outside of clang/test/Driver, tests should use `%clang_cc1` to

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

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

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

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

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

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

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

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

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

probinson added a reviewer: hokein. probinson added a subscriber: hokein. probinson added a comment. Herald added a project: All. + @hokein who has done work in the one place where `replaceNestedName` is used. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119040/new/

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

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

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

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

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

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

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

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

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

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

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

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

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

probinson added a subscriber: bkramer. probinson added a comment. Ping; + @bkramer who reviewed the original patch that added this test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119040/new/ https://reviews.llvm.org/D119040 ___

[PATCH] D111457: [test] Add lit helper for windows paths

probinson added inline comments. Comment at: llvm/utils/lit/lit/TestRunner.py:1124 +if kIsWindows: +fs_root = 'C:\\' +substitutions.extend([ rnk wrote: > It is pretty common to run the LLVM test suite on secondary drives, > especially on

[PATCH] D120066: [NFC] Fix debug-info-hotpatch.cpp failure due to downstream regex issue.

probinson added a comment. Seems to me there have been other cases where `.*` matching was greedier than expected, so this seems like the right fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120066/new/ https://reviews.llvm.org/D120066

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

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

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

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

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

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

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

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

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

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

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

probinson added a reviewer: kadircet. probinson added a subscriber: kadircet. probinson added a comment. Ping; +@kadircet CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119040/new/ https://reviews.llvm.org/D119040 ___ cfe-commits mailing list

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

probinson added a comment. > I think we should assume that PS4 will as well. Yes, please. We are essentially locked to the ABI as of clang 3.2. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117616/new/ https://reviews.llvm.org/D117616

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

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

[PATCH] D159054: [Driver] Removal of C_INCLUDE_DIRS feature

probinson added a comment. @MaskRay thanks for the info! In D159054#4638322 , @brad wrote: > I was going to hold this over for a bit longer. 2 weeks and if no one says > anything then go ahead? The main thing to worry about, clearly, is what happens

[PATCH] D159054: [Driver] Removal of C_INCLUDE_DIRS feature

probinson added a comment. In D159054#4626772 , @brad wrote: > Just FYI I am not in a rush to commit this. I am posting this more so as a > means of prodding for discussion of the feature. So far nobody has popped up to say they want it. @MaskRay I

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

probinson added a comment. > You need to do `returnit`. Doh! Thanks. Those two instantiations could have different function bodies, but would have the same mangled name. Got it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147655/new/

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

probinson added a comment. Hi @rsmith, > these two different templates would have the same mangling: template T returnit() {return I;}; template T returnit() { return I; } I tried compiling `long foo() { return returnit(); }` with these two templates, and got different manglings.

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGca1295c5a15f: [DebugInfo] Alternate (more efficient) MD5 fix (authored by probinson). Herald added a project: clang. Repository: rG LLVM Github

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. Detail added in the commit message, good idea! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156571/new/ https://reviews.llvm.org/D156571 ___ cfe-commits mailing list

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. This patch is possibly a suspect in at least some bot failures although I'm at a loss to understand why. Perhaps I can't just blithely call getChecksum() and copy what it sends back? The ways of metadata

[PATCH] D158614: [UBSan] Disable the function sanitizer on an execute-only target.

probinson added a comment. The PS5 bits LGTM, but as I'm not familiar with the ARM aspects I won't give final approval. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158614/new/ https://reviews.llvm.org/D158614

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156571/new/ https://reviews.llvm.org/D156571 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added a comment. Thanks @DavidSpickett the patch is currently reverted. I have a revised patch coming soon and I will keep a close eye on the bots. I believe it's a string-lifetime issue and so whether it manifests is unpredictable, but we have enough different bots in the farm that

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

probinson added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:423 + if (!CSInfo) { +SmallString<64> Checksum; +std::optional CSKind = In the final commit, `Checksum` is outside the `if` so that its lifetime persists to the end of the

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

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

[PATCH] D121299: [NVPTX] Disable DWARF .file directory for PTX

probinson added a comment. I'm okay with doing it this way. My impression over the years (as a debug-info guy, not as a ptxas user) is that ptxas evolves slowly and this is not really "busy work." Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596 + DefaultedSMFArePOD = false; + } + dblaikie wrote: > probinson wrote: > > Does this mean `-fclang-abi-compat` will override the PS4/Darwin special > > case? I think

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596 + DefaultedSMFArePOD = false; + } + dblaikie wrote: > probinson wrote: > > dblaikie wrote: > > > probinson wrote: > > > > Does this mean `-fclang-abi-compat` will

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added a comment. I'm in the middle of upstreaming PS5 support and I still didn't think of this... doh! Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5588 + bool DefaultedSMFArePOD = !RawTriple.isPS4() && !RawTriple.isOSDarwin(); + `isPS4()`

[PATCH] D126224: Add DWARF string debug to clang release notes.

probinson added a comment. I see some unrelated whitespace changes, we generally don't like mixing those with "real" changes. But the description seems fine to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126224/new/

[PATCH] D126224: Add DWARF string debug to clang release notes.

probinson added a comment. Unnamed variables are an oddity, sure; we've had to patch a downstream test or two that wasn't being careful enough. But not providing a name is entirely defensible, and consumers should be willing to cope with DWARF that doesn't fully meet their expectations.

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596 + DefaultedSMFArePOD = false; + } + Does this mean `-fclang-abi-compat` will override the PS4/Darwin special case? I think we don't want to do that. Repository:

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

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

[PATCH] D129404: Change default C dialect for PS5 to gnu17/gnu18.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129404/new/ https://reviews.llvm.org/D129404

[PATCH] D128934: [X86] Add RDPRU instruction

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

[PATCH] D126224: Add DWARF string debug to clang release notes.

probinson added a comment. In D126224#3532769 , @dblaikie wrote: > While the string would be deduplicated, the offset in the DIEs (or index in > DWARFv5, plus offset in .debug_str_offsets) for each of these strings would > not. But perhaps that would

[PATCH] D130493: Disable stack-sizes section by default for PS4.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130493/new/ https://reviews.llvm.org/D130493

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

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

[PATCH] D114115: [Driver] Support for compressed debug info on Fuchsia

probinson added a comment. Lit is aware of zlib's presence or absence, you could use `REQUIRES: zlib` (or you might factor that bit out into its own test with the REQUIRES). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114115/new/

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

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

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

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

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

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

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

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

[PATCH] D131919: Move googletest to the third-party directory

probinson added a comment. In D131919#3749850 , @Meinersbur wrote: > Semi-OT: `polly\lib\External` has 3 more third-party libraries. Two of them > have been heavily modified in-tree, the third has just a custom > CMakeLists.txt. > Should these

[PATCH] D131919: Move googletest to the third-party directory

probinson added inline comments. Comment at: clang/CMakeLists.txt:106 AND EXISTS ${UNITTEST_DIR}/CMakeLists.txt) add_subdirectory(${UNITTEST_DIR} utils/unittest) endif() Should this be `third-party/unittest` ? Looking at how the lldb

[PATCH] D134673: [Driver][PS4] pass -fcrash-diagnostics-dir to LTO

probinson added a comment. I'm guessing the non-PS4 targets would also want this, so a similar change is needed in tools::addLTOOptions. This can be a separate patch if you wish. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134673/new/

[PATCH] D134673: [Driver][PS4] pass -fcrash-diagnostics-dir to LTO

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134673/new/ https://reviews.llvm.org/D134673

[PATCH] D134507: [Clang] add missing ClangABICompat check

probinson added a subscriber: rjmccall. probinson added a comment. It feels odd to use a ClangABI check for something that is affecting what source is accepted, but this is not my area of expertise. @aaron.ballman or @rjmccall would probably be the right people to weigh in on this.

[PATCH] D134507: [Clang] add missing ClangABICompat check

probinson added a comment. In D134507#3818765 , @ychen wrote: > In D134507#3817928 , @probinson > wrote: > >> It feels odd to use a ClangABI check for something that is affecting what >> source is accepted, but

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

probinson added inline comments. Comment at: llvm/lib/IR/Verifier.cpp:3470 + // (Interposable functions are not inlinable as are functions w/o + // declarations). if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() && (Interposable

[PATCH] D136188: Update docs for -fuse-ctor-homing

probinson added a subscriber: jmorse. probinson added a comment. +jmorse who is closer to this topic than I am. We've had a few complaints from licensees about ctor homing, and debug-info size in general is something of a sensitive topic. But if we can come to a place where

[PATCH] D133998: [HIP][test] Avoid %T

probinson added subscribers: lamb-j, probinson. probinson added a comment. @MaskRay I see that you restored the `clang-driver` keyword in hip-link-bc-to-bc.hip, and of course that keyword is not defined anywhere so the test is never being run. Is there an issue filed to track this? Or is the

[PATCH] D136187: [clang][AIX] Omitting Explicit Debugger Tuning Option

probinson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4374 + ? llvm::DebuggerKind::Default + : DebuggerTuning); Seems like you should be able to return

[PATCH] D136309: [clang][Toolchains][Gnu] pass -g through to assembler

probinson added inline comments. Comment at: clang/test/Driver/as-options.s:121 +// Test that -g is passed through to GAS. +// RUN: %clang -fno-integrated-as -g %s -### 2>&1 | \ +// RUN: FileCheck --check-prefix=CHECK-DEBUG %s Please use an explicit triple

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

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

<    1   2   3   4   5   6   >