[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina added a comment. In D61756#2003836 , @MaskRay wrote: > > @rmisth wrote: "Clang should drive the standard, not diverge from it", and > > we should at least try to push for standardizing this if we think it's > > worthwhile as an alternative to

[PATCH] D75373: [Clang] Fix Hurd toolchain class hierarchy

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. In D75373#1900237 , @aganea wrote: > The patch did not make sense conceptually. Hurd is not Linux. I think now it > makes more sense. Yes this

[PATCH] D73845: [Gnu toolchain] Look at standard GCC multilib/multiarch paths by default

kristina added a comment. I feel like in this specific case it may be worth splitting this into two patches: - Factoring certain stuff out of `Linux.cpp` into `Gnu.(cpp|h)`. - Adding that machinery to `Hurd.cpp` with related tests. Also forgive me if I'm wrong but isn't Hurd only limited to

[PATCH] D72236: [Clang] Force rtlib=platform in test to avoid fails with CLANG_DEFAULT_RTLIB

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 rGce67db418537: [Clang] Force rtlib=platform in test to avoid fails with CLANG_DEFAULT_RTLIB (authored by kristina).

[PATCH] D72236: [Clang] Force rtlib=platform in test to avoid fails with CLANG_DEFAULT_RTLIB

kristina created this revision. kristina added reviewers: sammccall, lebedev.ri, sthibaul. Herald added a project: clang. Driver test `cross-linux.c` fails when CLANG_DEFAULT_RTLIB is "compiler-rt" as the it expects a GCC-style `"crtbegin.o"` after `"crti.o"` but instead receives something akin

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

kristina added a comment. For future - Since you seem to put up a lot of (well, almost all) patches related to Hurd support, may I suggest requesting commit access so you can land patches yourself after review? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit rGb18cb9c47166: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default (authored by kristina). Repository:

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

kristina accepted this revision. kristina added a comment. On second look this seems to be caused by `compiler-rt` being used as the default runtime, which is not accounted for in that particular test. Test is expecting a GCCesque crtbegin here which is missing with `compiler-rt`: "crt1.o"

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

kristina added a comment. I ran `check-clang` on a x86_64 Ubuntu 18.04 machine (toolchain compiled with `compiler-rt` as the default CRT and `libc++`/`libc++abi`/`libunwind_llvm`). I think one of the driver tests for multilib stuff may be broken, in which case it should probably be

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

kristina requested changes to this revision. kristina added a comment. Sorry for responding late, was away. This seems to be tripping up a regression test: FAIL: Clang :: Driver/cross-linux.c (5312 of 16608) llvm-project/clang/test/Driver/cross-linux.c:62:26: error: CHECK-MULTI32-X86-64:

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

kristina added inline comments. Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:2623 + int MaxVersion = 0; + std::string MaxVersionString = ""; + for (llvm::vfs::directory_iterator LI = vfs.dir_begin(base, EC), LE; sthibaul wrote: > kristina wrote: > > No

[PATCH] D69758: [Gnu toolchain] Look at standard GCC paths for libstdcxx by default

kristina added reviewers: mclow.lists, ldionne, EricWF, rsmith. kristina added a comment. Herald added a subscriber: dexonsmith. Added libcxx maintainers, would like one of them to sign off on this. I presume this is NFCI for Linux so it should be covered by existing regression tests?

[PATCH] D69754: [hurd] Add --build-id option when enabled

This revision was automatically updated to reflect the committed changes. Closed by commit rG79c89033fdf1: [Clang] Add ENABLE_LINKER_BUILD_ID to Hurd driver. (authored by kristina). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69754/new/

[PATCH] D69754: [hurd] Add --build-id option when enabled

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. LGTM, at least following the same reasoning as in rC271692 (http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160530/160984.html). I presume

[PATCH] D68410: [AttrDocs] document always_inline

kristina added a comment. Just linking relevant bug for the record: https://bugs.llvm.org/show_bug.cgi?id=43517 Also, I'm fairly certain `__forceinline` and `always_inline`, confusingly enough differ in semantics, with `__forceinline` only being a stronger hint on MSVC. Repository: rG

[PATCH] D62873: Avoid building analyzer plugins if CLANG_ENABLE_STATIC_ANALYZER is OFF

kristina added a comment. Experienced the same, updated my test build configuration to always force `CLANG_ENABLE_STATIC_ANALYZER` to On when building with tests. Maybe it's worth adding a warning about when Clang tests are being built? Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

kristina added a comment. In D61634#1517228 , @xbolva00 wrote: > I have a question about qsort.. If we provide own implementation of qsort and > replace calls to libc's qsort to our qsort, we could fully inline cmp > function then. Ideas? `qsort`

[PATCH] D61756: Add a __FILE_NAME__ macro.

This revision was automatically updated to reflect the committed changes. Closed by commit rC360938: Reland [Clang][PP] Add the __FILE_NAME__ builtin macro (authored by kristina, committed by ). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61756/new/

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina updated this revision to Diff 199882. kristina edited the summary of this revision. kristina added a comment. Revised to use `llvm::sys::path::filename` to avoid issues on Windows hosts. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61756/new/

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina reopened this revision. kristina added a comment. This revision is now accepted and ready to land. Reverted in rL360842 as Windows bots were failing. I suspect the `MSVCCompat` case may need to be handled differently depending on the host OS similar

[PATCH] D61756: Add a __FILE_NAME__ macro.

This revision was automatically updated to reflect the committed changes. Closed by commit rC360833: [Clang][PP] Add the __FILE_NAME__ builtin macro. (authored by kristina, committed by ). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61756/new/

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina added a comment. Landing this as discussed on IRC, will try to push it forward with WG14. I think having something like this as part of the standard would benefit a lot, since currently the "unofficial" way of doing this for either GCC or Clang involves several nonstandard builtins

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina added a comment. @rsmith Ping. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61756/new/ https://reviews.llvm.org/D61756 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina added a comment. Need @rsmith to bless this as it's introducing a nonstandard extension, however small it may be. The original diff did have a consensus on it, so I didn't really put up a formal RFC on `cfe-dev`. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina updated this revision to Diff 199104. kristina edited the summary of this revision. kristina added a comment. Actually I got it wrong, the path is normalized to use regular slashes at that point, so there is no point in handling backslashes in paths at all even with Microsoft

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina updated this revision to Diff 198894. kristina added a comment. Fix style, remove unnecessary braces, add missing newline. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61756/new/ https://reviews.llvm.org/D61756 Files:

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina abandoned this revision. kristina added a comment. Superseded by D61756 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17741/new/ https://reviews.llvm.org/D17741 ___ cfe-commits mailing list

[PATCH] D61756: Add a __FILE_NAME__ macro.

kristina created this revision. kristina added reviewers: aaron.ballman, rsmith, rnk. Herald added a project: clang. A much simplified version of D17741 which adds a new builtin macro `__FILE_NAME__` that is similar to `__FILE__` but only renders the last path

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina commandeered this revision. kristina added a reviewer: weimingz. kristina added a comment. Sorry, forgot about this, will make a new diff with just the macro for review later tonight. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17741/new/ https://reviews.llvm.org/D17741

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina added a comment. If the author is still missing at the end of next week, any objections to me resubmitting a similar patch that just implements `__FILE_NAME__` or `__BASE_NAME__` (Need a few more opinions here I guess, personally I think `__FILE_NAME__` makes more sense)? I'll carve

[PATCH] D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language

kristina added a comment. The patch itself looks sound. However given that you have a specific use case in mind (TableGen files) could you provide supplementary coverage for that specific use case (unit tests for formatting `td` syntax using

[PATCH] D56241: [Sema] expose a control flag for integer to pointer ext warning

This revision was automatically updated to reflect the committed changes. Closed by commit rC351082: [Sema] Expose a control flag for integer to pointer ext warning (authored by kristina, committed by ). Changed prior to commit: https://reviews.llvm.org/D56241?vs=180143=181585#toc Repository:

[PATCH] D56241: expose a control flag for interger to pointer ext warning

kristina added a comment. Going to test and land it as requested by @lebedev.ri on IRC. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56241/new/ https://reviews.llvm.org/D56241 ___ cfe-commits mailing list

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina added a comment. Ping for author? This is one of the changes I'd like to see through, though the complexity here can be massively reduced as stated above. I wouldn't mind opening a new diff if the author is away with just a change in PPMacroExpansion and a testcase (which should

[PATCH] D56415: NFC: Port QueryParser to StringRef

kristina added a reviewer: kristina. kristina added a comment. LGTM aside from one comment. Comment at: clang-query/QueryParser.cpp:36 + if (Line.front() == '#') { +Line = {}; return StringRef(); I don't think this is the best way of handling it, in

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina added a comment. I should add that this is not just about reproducible builds but about code size as mentioned by @NSProgrammer. There are ways to cheat with builtins but the problem is, entire __FILE__ string is still emitted into read-only data, despite the constexpressiveness of

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina added a comment. In D17741#1345171 , @NSProgrammer wrote: > To throw in my 2 cents. I don’t really have a preference between a compiler > flag vs a different macro that’s just for the file name sans path prefix. > But I have a real need for

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

kristina added a comment. I like the idea of just getting the filename reliably, but I think an extension to strip path prefixes is a bit of an overkill especially as yet another compiler flag. I implemented a similar thing in my fork in form of a directive to `__generate(...)` which is my own

[PATCH] D55953: Android is not GNU, so don't claim that it is.

kristina added a comment. Seems good, it could eliminate the need for a lot of preprocessor checks like `#if __gnu_linux__ && !defined(__ANDROID__)`. It doesn't seem that there are any preprocessor checks where this would cause problems (from a quick search) since all of them seem to focus on

[PATCH] D55734: [analyzer] Refactoring GenericTaintChecker.cpp

kristina added a comment. If you don't mind, please submit diffs with full context (`diff -U9` or something alike depending on what you use). It makes it much easier to review patches. Also the formatting looks really off in some places where 4 spaces are used. Also another nitpick, nested

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina requested changes to this revision. kristina added a comment. In D55150#1321829 , @efriedma wrote: > I'm not sure that putting a warning that can be disabled really helps here; > anyone who needs the option will just disable the warning anyway,

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina added a comment. In D55150#1321734 , @thakis wrote: > I don't have an opinion on this patch (if you force me to have one, I'm > weakly in favor), but I agree with the general sentiment. When I told people > to not use mllvm and Xclang before,

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina added a comment. > There is a cost to having people encode these flags into their build systems > -- it can then cause issues if we ever change these internal flags. I do not > think any Clang maintainer intends to support these as stable APIs, unlike > most of the driver

[PATCH] D55150: Emit warnings from the driver for use of -mllvm or -Xclang options.

kristina added a comment. Personally I'm against this type of warning as it's likely anyone using `-mllvm` is actually intending to adjust certain behaviors across one or more passes with a lot of switches supported by it being intentionally hidden from ` --help` output requiring explicitly

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

This revision was automatically updated to reflect the committed changes. Closed by commit rL348368: [Haiku] Support __float128 for x86 and x86_64 (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D54379: Add Hurd toolchain support to Clang

This revision was automatically updated to reflect the committed changes. Closed by commit rC347833: Add Hurd target to Clang driver (2/2) (authored by kristina, committed by ). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. Yes everything passes now, my bad for not noticing. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina accepted this revision as: kristina. kristina added a comment. Actually nevermind, I'll just create them myself, seems like a strange bug. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Hm, yes you're right, the files aren't in the diff that Phab gives me, this is annoying. I wonder if it's stripping them from the diff for some reason, can you upload the raw diff? Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Your tests don't pass: Testing Time: 21.51s Failing Tests (1): Clang :: Driver/hurd.c Expected Passes: 470 Expected Failures : 3 Unsupported Tests : 71 Unexpected Failures: 1 Seems to be

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a comment. Let me know if you need this committed. Comment at: lib/Basic/Targets/OSTargets.h:260 DefineStd(Builder, "unix", Opts); +if (this->HasFloat128) { + Builder.defineMacro("__FLOAT128__"); One tiny style-related nitpick, no

[PATCH] D54901: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina accepted this revision. kristina added a comment. This revision is now accepted and ready to land. LGTM. (Revision of D53696 ) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54901/new/ https://reviews.llvm.org/D54901

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina commandeered this revision. kristina edited reviewers, added: return; removed: kristina. kristina added a comment. New revision in D54901 . Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53696/new/

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Alright, will patch it into my fork in a bit and try it out, if everything looks good, I'll land the stack. (Again huge sorry about this taking some time, have lots on my hands at the moment) Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a comment. Ping. Is the author still around? I'm happy to take over this revision if not, and add `__FLOAT128__` but unless someone says `_GLIBCXX_USE_FLOAT128` is actually defined by the toolchain, I'll remove that line, it looks like it really shouldn't be defined in the

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Ping. Would like someone else to co-review this, everything looks fine aside from tests (I think a portion of this could use a short unit test with regards to creating the actual target instance). Also your FileCheck test is only for invoking `clang -cc1` from the

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina requested changes to this revision. kristina added a comment. In https://reviews.llvm.org/D53696#1305990, @kallisti5 wrote: > Indeed. I agree the _GLIBCXX_USE_FLOAT128 is misplaced there. That comes > from config.h in libstdc++. That's working around an issue in Haiku's build >

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a comment. Linux, Solaris and OpenBSD also define it where appropriate, it depends on the target. This is why it's much easier to review when full context (diff with `-U 9`) is provided, and means that it has a much lower change of `patch` doing something unexpected.

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

kristina added a reviewer: kristina. kristina added a comment. Do you mind resubmitting those with context? Also I would suggest asking Tom Stellard as he's in charge of handling cherrypicking patches to go into releases once the major rolls over and I think we're pretty close (?) to 7.0.1.

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. I'll re-review when I'm up, from a quick glance it looks much better but I'll have to patch it over my fork and try out a few things (Mostly x86_64 Linux and Darwin test suites). I think the test is lacking a bit, there's a lot of stuff that isn't covered, and there's

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. As discussed in `#hurd`, a few additional comments. The Hurd codepath should first check if the triple is eligible, ie. minimizing any cost to the driver invocation for most targets, which are not going to be Hurd. In fact I would wrap it in `LLVM_UNLIKELY` but that's

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Also, this needs unit tests and FileCheck tests. Repository: rC Clang https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:418 + replaceString(T, "-pc-gnu", "-pc-hurd-gnu"); + TargetTriple = T; + kristina wrote: > Reference to a local variable? Hm, actually this is fine I guess, just avoid `strlen` and pass literals

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:418 + replaceString(T, "-pc-gnu", "-pc-hurd-gnu"); + TargetTriple = T; + Reference to a local variable? Repository: rC Clang https://reviews.llvm.org/D54379

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Commented on that particular idiom, there's two instances where it's used, aside from variable naming issues (`pos` should be `Pos`) it's very non idiomatic as far as rest of LLVM code goes, don't pass string literals around as `const char*`, prefer `StringRef`

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. The other lost comment was regarding the functions where you're using `strcpy` instead of idiomatic LLVM and also creating unnecessary temporary `std::string` instances on the stack. Repository: rC Clang https://reviews.llvm.org/D54379

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Phab lost this inline command for some reason, but please leave some comment regarding why that part in `Driver.cpp` does what it does (summarize the conclusion of the discussion in https://reviews.llvm.org/D54378). Comment at:

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added inline comments. Comment at: lib/Driver/Driver.cpp:392 +static void replaceString(std::string , + const char *Other, Same as previous comment regarding this type of function. Repository: rC Clang

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added a comment. Added first batch of comments regarding the patch. Some style, some semantics-related. Comment at: lib/Basic/Targets/OSTargets.h:283 +Builder.defineMacro("__GLIBC__"); +Builder.defineMacro("__ELF__"); +if (Opts.POSIXThreads)

[PATCH] D54379: Add Hurd toolchain support to Clang

kristina added reviewers: aaron.ballman, erik.pilkington, rsmith. kristina added a comment. Alright, once this is fully reviewed, if accepted, I can land the LLVMSupport change and add your new target and close the stack. In the meantime, adding some more reviewers who have more experience with

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

This revision was automatically updated to reflect the committed changes. Closed by commit rL346628: [CodeGen][CXX]: Fix no_destroy CG bug under specific circumstances (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. Though on the other hand it makes no sense to skip it with asserts off either, that just clicked in my head, sorry. Repository: rC Clang https://reviews.llvm.org/D54344 ___ cfe-commits mailing list

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173602. kristina added a comment. Revised to address comments. Repository: rC Clang https://reviews.llvm.org/D54344 Files: lib/CodeGen/CGDeclCXX.cpp test/CodeGenCXX/attr-no-destroy-d54344.cpp Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. In https://reviews.llvm.org/D54344#1294942, @erik.pilkington wrote: > LGTM too, with one small fix in test. Thanks for working on this! Well, I figured since the assertion being tripped was (before investigation) the only reliable way to notice the bug it makes sense

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

kristina added a comment. In https://reviews.llvm.org/D53263#1294488, @JDevlieghere wrote: > The patch applies for me but has quite a few style violations. I'll fix those > up before landing it. Thank you and sorry for the trouble, my fork seems to have enough modifications to some of these

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

kristina added a comment. In https://reviews.llvm.org/D53263#1294412, @bobsayshilol wrote: > In https://reviews.llvm.org/D53263#1294383, @kristina wrote: > > > I can do it if you'd like, will be a moment though. > > > Thanks and much appreciated! Huge apologies, it seems I can't get this to

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173534. kristina added a comment. Revised to address nitpicks. Repository: rC Clang https://reviews.llvm.org/D54344 Files: lib/CodeGen/CGDeclCXX.cpp test/CodeGenCXX/attr-no-destroy-d54344.cpp Index: test/CodeGenCXX/attr-no-destroy-d54344.cpp

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

kristina added a comment. In https://reviews.llvm.org/D53263#1294336, @bobsayshilol wrote: > Thanks! > I don't have commit access to land this myself. I can do it if you'd like, will be a moment though. https://reviews.llvm.org/D53263 ___

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173530. kristina set the repository for this revision to rC Clang. kristina added a comment. @erik.pilkington Fantastic catch, I totally forgot about the global flag. Revised, moved to CodeGenCXX, added a test to verify ctor/dtor is balanced under normal

[PATCH] D54379: Add Hurd support

kristina added reviewers: kristina, clang. kristina added a comment. A few style naming/comments. Comment at: lib/Driver/ToolChains/Hurd.cpp:36 + // clever. + switch (TargetTriple.getArch()) { + default: Does this need a switch? Wouldn't an `if` be

[PATCH] D54379: Add Hurd support

kristina added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:533 - if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI) { + if (Triple.isOSLinux() || Triple.getOS() == llvm::Triple::CloudABI || Triple.isOSHurd()) { switch

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173507. kristina added a comment. Revised, added a newline to the testcase. https://reviews.llvm.org/D54344 Files: lib/CodeGen/CGDeclCXX.cpp test/SemaCXX/attr-no-destroy-d54344.cpp Index: test/SemaCXX/attr-no-destroy-d54344.cpp

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173503. kristina set the repository for this revision to rC Clang. kristina added a comment. Revised, I think I worked out what was happening, there's an explanation in the comment in the test. This is a relatively new attribute so the coverage for it did

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. I've managed to isolate enough to make for a testcase. Is that too broad or is it okay to attach as is? The following triggers the assert: namespace util { template class test_vector { public: test_vector(unsigned c)

[PATCH] D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.

This revision was automatically updated to reflect the committed changes. Closed by commit rL346582: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC. (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.

kristina created this revision. kristina added a reviewer: clang. Herald added a subscriber: cfe-commits. Noticed while working with that area of code, NFC. Repository: rC Clang https://reviews.llvm.org/D54373 Files: lib/CodeGen/CGDeclCXX.cpp Index: lib/CodeGen/CGDeclCXX.cpp

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. This seems to be a peculiar side effect of weird combinations of previous uses of attributes `no_destroy`, `used`, and `section`, as well as complex macros used to create metaclass-like systems (through the use of said attributes). It seems that there is a serious bug

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. In https://reviews.llvm.org/D54344#1293643, @aaron.ballman wrote: > In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote: > > > Have you tried running creduce on the preprocessed source? We should really > > have a real reproducer for this, otherwise its

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina added a comment. In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote: > Have you tried running creduce on the preprocessed source? We should really > have a real reproducer for this, otherwise its really hard to tell what the > underlying problem is. Unable to build

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina updated this revision to Diff 173411. kristina added a comment. Revised (style/ordering). https://reviews.llvm.org/D54344 Files: lib/CodeGen/CGDeclCXX.cpp Index: lib/CodeGen/CGDeclCXX.cpp === ---

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

kristina created this revision. kristina added reviewers: rsmith, clang, JonasToth, EricWF, aaron.ballman. Herald added subscribers: dexonsmith, mehdi_amini. Herald added a reviewer: jfb. Difficult to reproduce crash, attempted it in the test, it appears to not trigger it. I can consistently

[PATCH] D53475: Create ConstantExpr class

kristina added a comment. Ah, it was causing this warning during build: /SourceCache/llvm-trunk-8.0/tools/clang/include/clang/AST/Expr.h:903:1: warning: 'ConstantExpr' defined as a struct here but previously declared as a class [-Wmismatched-tags] Repository: rC Clang

[PATCH] D53475: Create ConstantExpr class

kristina added a comment. I think you may have accidentally commited the wrong patch. +struct ConstantExpr : public FullExpr { Is causing a warning right now, not sure where that came from. Repository: rC Clang https://reviews.llvm.org/D53475

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

This revision was automatically updated to reflect the committed changes. Closed by commit rC344739: Add support for -mno-tls-direct-seg-refs to Clang (authored by kristina, committed by ). Changed prior to commit: https://reviews.llvm.org/D53102?vs=169224=170082#toc Repository: rC Clang

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

kristina added a comment. By the way, out of curiosity is this for anything specific (alternative libc or some user-mode-scheduling implementation)? Not nitpicking, just curious since it's an interesting topic in general and it's frustrating that the architecture is so limited in terms of

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

kristina added a comment. If the author doesn't mind I can just apply the style fix after patching and then rebuild and run all the relevant tests (or would you prefer to do that?). Seems easier than a new revision for an indentation change on one line. https://reviews.llvm.org/D53102

[PATCH] D53102: Support for the mno-tls-direct-seg-refs flag

kristina added a comment. @nruslan This patchset is still pending review so be patient, I landed https://reviews.llvm.org/D53103 as it was reviewed and accepted by the code owner, on which this patch depends on. That said it LGTM, it's simple enough as it just forwards the argument to the

[PATCH] D52774: [Preprocessor] Fix an assertion failure trigged in clangd #include completion.

kristina added a reviewer: kristina. kristina added a comment. Could you add a regression test please? Thank you. Repository: rC Clang https://reviews.llvm.org/D52774 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D52721: [Preprocessor] Fix a crash when handling non-alpha include header.

kristina added a comment. Please make sure it builds after you update the revision, I usually do it myself but it'll take too long on my desktop and I accidentally broke the hypervisor on my buildbot so can't do it this time. Comment at: lib/Lex/PPDirectives.cpp:1898 +

[PATCH] D52721: [Preprocessor] Fix a crash when handling non-alpha include header.

kristina accepted this revision. kristina added a comment. Yes I think the clangd crash will have to go in another diff, but this fixes the crash in clang, which is pretty good in itself. I don't build clangd at the moment and have no buildbot available so I can try to look into it later if you

  1   2   >