[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

grimar added a comment. I like this approach. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95246/new/ https://reviews.llvm.org/D95246 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D95246: [SystemZ][z/OS] Fix No such file or directory expression error matching in lit tests - continued

grimar added a comment. As far I understand, looking on the description of D94239 , the message on z/OS looks like "EDC5129I No such file or directory.". I guess the `EDC5129I` is a stable error code? So why not to check for a possible `EDC5129I` prefix word ins

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

grimar accepted this revision. grimar added a comment. This revision is now accepted and ready to land. LGTM. It worth wainting for a second approvement and/or other comments to verify that people are happy with doing this for `clang-format`. Comment at: clang/unittests/Format

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

grimar added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:16079 + auto Style9 = getStyle("file", "/d/.clang-format", "LLVM", "", &FS, true); + ASSERT_TRUE((bool)Style9); } It looks like it is very similar to "Test 7:" and can be a part of

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

grimar added inline comments. Comment at: llvm/lib/Support/YAMLTraits.cpp:204 if (!is_contained(MN->ValidKeys, NN.first())) { - setError(NN.second.get(), Twine("unknown key '") + NN.first() + "'"); - break; + auto ReportNode = NN.second.get(); + auto Repo

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

grimar added a comment. I am not familar with `clang-format`, but have a few comments inlined about the rest. I think the new `setIgnoreUnknown` YAMLlib API is probably OK generally. I'd perhaps call it differently, e.g. `setAllowUnknownKeys` though. Also, I think you need to add a unit testing

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

grimar added a comment. In D78024#2087800 , @Higuoxing wrote: > > btw, do you know why FileCheck seems intentionally allows the case when > > --check-prefixes=KNOWN,UNKNOWN? > > I've mentioned in the description of D78110 > >

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

grimar added a comment. In D78024#1989287 , @probinson wrote: > I'm just reading this review for the first time, and my thought was, why is > this interacting with implicit-check-not? It was interacting (this patch was committed and fixed the issue), be

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

grimar marked an inline comment as done. grimar added inline comments. Comment at: llvm/lib/Support/FileCheck.cpp:1375-1377 + // We do not allow using -implicit-check-not when an explicitly specified + // check prefix is not present in the input buffer. + if ((Req.IsDefaultChe

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

grimar marked an inline comment as done. grimar added inline comments. Comment at: llvm/lib/Support/FileCheck.cpp:1375-1377 + // We do not allow using -implicit-check-not when an explicitly specified + // check prefix is not present in the input buffer. + if ((Req.IsDefaultChe

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG65a2de7e6c98: [FileCheck] - Fix the false positive when -implicit-check-not is used with an… (authored by grimar). Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed prior to c

[PATCH] D78016: [ADT/STLExtras.h] - Add llvm::is_sorted wrapper and update callers.

This revision was automatically updated to reflect the committed changes. Closed by commit rG1647ff6e2753: [ADT/STLExtras.h] - Add llvm::is_sorted wrapper and update callers. (authored by grimar). Herald added subscribers: cfe-commits, jrtc27. Herald added a project: clang. Repository: rG LLVM

[PATCH] D77484: [Vector] Pass VectLib to LTO backend so TLI build correct vector function list

grimar added inline comments. Comment at: lld/ELF/Options.td:491 HelpText<"Sample profile file path">; +def lto_vector_library: J<"lto-vector-library=">, + HelpText<"Vector functions library">; You need to update the documentation either (`lld/docs/ld.lld.1`)

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

grimar added a comment. In D77184#1960437 , @thakis wrote: > grimar, andrewng: You both have checkout and build on different drives too, > yes? Nope. I think my configuration is just normal, except I do not use "C:". My checkout is "D:\Work3\LLVM\llvm-p

[PATCH] D77184: Make it possible for lit.site.cfg to contain relative paths, and use it for llvm and clang

grimar added a comment. In D77184#1959351 , @andrewng wrote: > The following patch fixes my issues on Windows, but I haven't tested that it > doesn't break anything else: > > diff --git a/llvm/cmake/modules/AddLLVM.cmake > b/llvm/cmake/modules/AddLLVM.

[PATCH] D68049: Propeller: Clang options for basic block sections

grimar added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:120 + std::string BasicBlockSections; + MaskRay wrote: > Comment its allowed values ("all", "labels", "none") I'd suggest to rewrite it somehow. This set of values did not hel

[PATCH] D72829: Implement -fsemantic-interposition

grimar added inline comments. Comment at: llvm/lib/IR/Module.cpp:558 +bool Module::getSemanticInterposition() const { + auto *Val = + cast_or_null(getModuleFlag("SemanticInterposition")); A minor nit about style: This probably would be nicer as ``` if (a

[PATCH] D69043: [RFC] Adding time-trace to LLD?

grimar added a comment. In D69043#1714134 , @russell.gallop wrote: > In D69043#1714060 , @ruiu wrote: > > > (I couldn't download your file because the server times out perhaps due to > > the file size.) > > > That

[PATCH] D65564: Improve raw_ostream so that you can "write" colors using operator<

grimar added a comment. It looks good to me. A minor nit is inlined. Comment at: clang/tools/diagtool/TreeView.cpp:30 - TreePrinter(llvm::raw_ostream &out) - : out(out), ShowColors(hasColors(out)), Internal(false) {} - - void setColor(llvm::raw_ostream::Colors Color) {

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar added a comment. Herald added a subscriber: jdoerfert. Herald added a project: LLVM. I am planning to revert it because of https://bugs.llvm.org/show_bug.cgi?id=41161 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55006/new/ https://reviews.llvm.org/D55006

[PATCH] D56927: Disable PIC/PIE for MSP430 target

grimar added a comment. Ok, please ignore my comments above. I applied the patch and found it creates the test in `llvm\tools\clang\test\CodeGen` and not in `llvm\test\CodeGen`. Was confused by phab, sorry for the noise. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D56927: Disable PIC/PIE for MSP430 target

grimar added a comment. Oh sorry, I looked in the wrong folder, my previous comment was not correct. So, this patch adds a test to `test\CodeGen` when we have `test\CodeGen\MSP430` folder. And `test\CodeGen` does not have any tests now at all. So I **guess** the correct way would probably be: 1

[PATCH] D56927: Disable PIC/PIE for MSP430 target

grimar added a comment. What I noticed, this seems to introduce the first test case in `test\CodeGen\MSP430` folder which uses `clang`. All the others use `llc`. Is it OK? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56927/new/ https://reviews.llvm.org/D56927

[PATCH] D56925: Do not use frame pointer by default for MSP430

grimar added a comment. Thanks for adding a test case! I know not enough about msp430/clang to verify it, unfortunately. (I just asked for a test because it is always useful to have one for any non-NFC patch). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56925/

[PATCH] D56927: Disable PIC/PIE for MSP430 target

grimar added a comment. Can it be tested with a test case? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56927/new/ https://reviews.llvm.org/D56927 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists

[PATCH] D56925: Do not use frame pointer by default for MSP430

grimar added a comment. Testcase? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56925/new/ https://reviews.llvm.org/D56925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D56663: [MSP430] Improve support of 'interrupt' attribute

grimar added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:5381 + // MSP430 'interrupt' attribute is applied to + // a function with no parameters and void return type + if (!isFunctionOrMethod(D)) { nit: code comments usually have a dot `.` at the en

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

This revision was automatically updated to reflect the committed changes. Closed by commit rL348352: [clang] - Simplify tools::SplitDebugName. (authored by grimar, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D55006?vs=175843&id=1767

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar added a comment. In D55006#1316985 , @JonasToth wrote: > from my side no objections, mailing list did not react AFAIK (just in case > your waiting for me until you recommit). David requested the test case, I added it and now waiting for his appro

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar updated this revision to Diff 175843. grimar added a comment. - Added the test case. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55006/new/ https://reviews.llvm.org/D55006 Files: lib/Driver/ToolChains/Clang.cpp lib/Driver/ToolChains/CommonArgs.cpp lib/Driver/ToolChains/C

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar added a comment. In D55006#1312012 , @JonasToth wrote: > The issues seems to be resolved for me as well, i will post this patch to the > mailing list and ask the other guy if his build is fine, too. Thanks, Jonas! CHANGES SINCE LAST ACTION ht

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar added a comment. In D55006#1311400 , @dblaikie wrote: > In D55006#1311398 , @grimar wrote: > > > In D55006#1311391 , @dblaikie > > wrote: > > > > > Cool - thanks! Any

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar added a comment. In D55006#1311391 , @dblaikie wrote: > Cool - thanks! Any chance/way to add a test for this that'd show up sooner > than the breakage caused by the previous version? Maybe, I am not sure. I was not able to trigger this without us

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar added a comment. With that the original issue mentioned in the following comment is fixed for me: https://reviews.llvm.org/rL347035#299232 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55006/new/ https://reviews.llvm.org/D55006 ___ c

[PATCH] D55006: [clang] - Simplify tools::SplitDebugName

grimar created this revision. grimar added reviewers: dblaikie, JonasToth. This is an updated version of the D54576 , which was reverted. Problem was that `SplitDebugName` calls the `InputInfo::getFilename` which asserts if `InputInfo` given is not of type `File

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

This revision was automatically updated to reflect the committed changes. Closed by commit rL347035: [clang] - Simplify tools::SplitDebugName. (authored by grimar, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D54576?vs=174193&id=1743

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

grimar created this revision. grimar added a reviewer: dblaikie. grimar added a comment. `SplitDebugName` is called from the following 4 places: 1. void Clang::ConstructJob() (https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L3933) We can get here when clang cons

[PATCH] D54576: [clang] - Simplify tools::SplitDebugName.

grimar added a comment. `SplitDebugName` is called from the following 4 places: 1. void Clang::ConstructJob() (https://github.com/llvm-mirror/clang/blob/master/lib/Driver/ToolChains/Clang.cpp#L3933) We can get here when clang construct jobs for regular case (cpp files): EX: main.cpp -g -gsplit-

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

This revision was automatically updated to reflect the committed changes. Closed by commit rC346837: [Clang] - Add '-gsplit-dwarf[=split,=single]' version for '-gsplit-dwarf'… (authored by grimar, committed by ). Changed prior to commit: https://reviews.llvm.org/D52296?vs=173643&id=173998#toc

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

grimar added inline comments. Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830 + if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ)) +if (StringRef(A->getValue()) == "single") + return Args.MakeArgString(Output.getFilename()); + Arg *FinalOutput = Arg

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

grimar added inline comments. Comment at: lib/Driver/ToolChains/CommonArgs.cpp:813-830 + if (Arg *A = Args.getLastArg(options::OPT_gsplit_dwarf_EQ)) +if (StringRef(A->getValue()) == "single") + return Args.MakeArgString(Output.getFilename()); + Arg *FinalOutput = Arg

[PATCH] D52296: [Clang] - Add '-gsplit-dwarf[=split, =single]' version for '-gsplit-dwarf' option.

grimar updated this revision to Diff 173643. grimar retitled this revision from "[Clang] - Add -fdwarf-fission=split,single option." to "[Clang] - Add '-gsplit-dwarf[=split,=single]' version for '-gsplit-dwarf' option.". grimar edited the summary of this revision. grimar added a comment. Thanks,

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

grimar 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.is

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

grimar added a comment. Looks like this behavior is what clang already have atm. Messages for the options that use `D.Diag` to report invalid values can be printed multiple times sometimes. The example is below: clang main.cpp -fdwarf-fission=foo -o 1.o -mthread-model bar -fcf-runtime-abi=z

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

grimar 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.is

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

grimar 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.is

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

grimar added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:2999-3001 +StringRef Value = A->getOption().matches(options::OPT_fdwarf_fission_EQ) + ? A->getValue() + : "split"; dblaikie wrote: > Ra

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

grimar updated this revision to Diff 173135. grimar marked an inline comment as done. grimar added a comment. - Addressed review comments. https://reviews.llvm.org/D52296 Files: include/clang/Driver/CC1Options.td include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.def

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

grimar added a comment. In https://reviews.llvm.org/D52296#1290212, @dblaikie wrote: > Thanks! - though on reflection I'm going to invoke @echristo again about the > naming. It's unfortunately a bit backwards that the pre-standard flag is > -gsplit-dwarf and what we're proposing as the standard

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

grimar updated this revision to Diff 172953. grimar retitled this revision from "[Clang] - Add -gsingle-file-split-dwarf option." to "[Clang] - Add -fdwarf-fission=split,single option.". grimar added a comment. Reimplemented option as `-fdwarf-fission[=split,single]`. https://reviews.llvm.org/D

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

grimar added a comment. In https://reviews.llvm.org/D52296#1284130, @probinson wrote: > 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

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

grimar added a comment. 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` as an alias for `-gsplit-dwarf`. And `-fsingle-file-debug-fission` for the new

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

grimar added a comment. In https://reviews.llvm.org/D52296#1281642, @echristo wrote: > Can you elaborate on your motivations and what you're trying to do? > > Thanks! We want to see: - No extra files. The compiler produces just a .o. - The linker leaves most debug info in the .o files. That m

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

grimar updated this revision to Diff 171480. grimar added a comment. Ping. - Rebased. https://reviews.llvm.org/D52296 Files: include/clang/Driver/CC1Options.td include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.h lib/CodeGen/BackendUtil.cpp lib/Driver/ToolChains/Cl

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

grimar added a comment. Maybe `-gno-dwo`? So we would write `-genable-split-dwarf -gno-dwo`? https://reviews.llvm.org/D52296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

grimar added a comment. In https://reviews.llvm.org/D52296#1258677, @alexshap wrote: > @grimar, this is an interesting observation which I've had on my mind for > quite some time as well; a couple of things which I have not double-checked > yet - just in case - do both gold and lld completely i

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

grimar added a comment. Ping. https://reviews.llvm.org/D52296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

grimar added a comment. Ping. https://reviews.llvm.org/D52296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

grimar added a comment. In https://reviews.llvm.org/D52296#1243688, @echristo wrote: > In https://reviews.llvm.org/D52296#1241928, @probinson wrote: > > > 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 th

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

grimar added a comment. To summarise: - It shares most of the benefits with the .dwo solution. - Unlike .dwo, there is no need to split the file and it is friendlier to other tools (ccache, distcc, etc) - But unlike .dwo a distributed build system has to copy larger .o files. https://reviews.l

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

grimar added a comment. We want to take the benefit that debug fission provides, but keep the debug information in the objects. That is a bit less intrusive way than using regular split dwarf flow. For a regular build and debug cycle that allows the linker to do minimal processing of the .debug

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

grimar created this revision. grimar added reviewers: dblaikie, echristo, probinson, compnerd. Herald added subscribers: JDevlieghere, aprantl. The DWARF5 specification says(Appendix F.1): "The sections that do not require relocation, however, **can be written to the relocatable object (.o) file

[PATCH] D28807: [Clang] - Update code to match upcoming llvm::zlib API.

This revision was automatically updated to reflect the committed changes. Closed by commit rL292227: [Clang] - Update code to match upcoming llvm::zlib API. (authored by grimar). Changed prior to commit: https://reviews.llvm.org/D28807?vs=84669&id=84676#toc Repository: rL LLVM https://revie

[PATCH] D28807: [Clang] - Update code to match upcoming llvm::zlib API.

grimar created this revision. https://reviews.llvm.org/D28684 changed llvm::zlib to return Error instead of Status. It was accepted and committed in r292214, but then reverted in r292217 because I missed that clang code also needs to be updated. Patch do that. https://reviews.llvm.org/D28807