[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina created this revision. kristina added reviewers: rnk, echristo, clang, rjmccall. Herald added a subscriber: cfe-commits. Attempt to unbreak behavior caused by https://reviews.llvm.org/D44491 resulting in inability to build standalone/unbridged CoreFoundation on an ELF target with `-fcon

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina added a comment. Does appear to work properly: cfstring:002AC640 off_2AC640 dq offset __CFConstantStringClassReference cfstring:002AC640 ; DATA XREF: skipDTD+19D↑o cfstring:002AC640

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina added a comment. Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so it's in line with ELF section naming conventions (I'm not entirely sure if that could cause issues with ObjC stuff)? And yes I think it's best to avoid that code-path altogether if it turns out to

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina updated this revision to Diff 166576. kristina added a comment. Does this look about right? I'm not sure what's up with DSO local attribute, it doesn't seem to apply, I think this test makes sense though. `ninja check-clang-codegen` succeeded this time: Testing Time: 12.77s Expec

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina marked 2 inline comments as done. kristina added a comment. Binary layout also looks sane (compiled with `-fPIC -faddrsig -Wl,--icf=safe`), just digging it through it in a dissasembler: [/q/src/clwn]$ llvm-readobj -elf-output-style=GNU -sections /q/org.llvm.caches/clownschool/clwn-sy

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina marked 3 inline comments as done. kristina added a comment. Want to see what @rnk has to say about this before landing it since he wrote the original code and if my understanding of `common` vs `dso_local` is accurate or not since I don't have much experience with the Windows specific

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

kristina added a comment. I think on Darwin it would **not** make sense to have `libc++fs.a` ship in `libc++.dylib` especially considering that it ends up in the dyld cache and that has a lot of other implications. It would make sense to ship it as a separate library, perhaps as part of the SDK

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina added a comment. In https://reviews.llvm.org/D52344#1243149, @rjmccall wrote: > I can respect wanting to change the rules on ELF, but it sounds like we do > need to stick with the current section names. Absent an ABI break to stop > looking for the existing section names, CF will misb

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina closed this revision. kristina added a comment. Closed by https://reviews.llvm.org/rL342883 (https://reviews.llvm.org/rC342883). Manually closing it as Phabricator is slightly broken, hopefully it makes the links when it catches up. Repository: rC Clang https://reviews.llvm.org/D52

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina added a comment. Alright, I guess I'll land it if there's no objections to it, `cfstring` is staying as is, the PE/COFF codepath is not affected in terms of functionality and test is fine I think, hopefully `x86_64-elf` will not yield different results on different machines in terms of

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina reopened this revision. kristina added a comment. This revision is now accepted and ready to land. Cascade of build failures stemming from `GV->setSection(".rodata");`, reverted the commit, it seems that `CFString.c` is causing all those issues despite passing when ran locally. Reposi

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina added a comment. Turns out test didn't execute as there are two tests named `CFString.c` and `cfstring.c` which usually shouldn't matter unless LLVM source checkout is done into a case insensitive filesystem in which case the failing test is skipped, this seems like a bug, going to ren

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

kristina updated this revision to Diff 166996. kristina added a comment. Split up the tests for ELF into tests 1). Checking whether CF can be built and linked correctly 2). Asm section tests originally from `CFStrings.c` as they had additional ELF-related cruft that should have been a separate t

[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

This revision was automatically updated to reflect the committed changes. Closed by commit rL343038: Reland "[Clang][CodeGen][ObjC]: Fix CoreFoundation on ELF with `-fconstant… (authored by kristina, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://revie

[PATCH] D52660: [CMake][Fuchsia] Use libc++ ABIv2 for Fuchsia toolchain

kristina added a comment. LGTM. Can we formalize ABIv2 as stable and make it distinct from unstable (soon to be ABIv3) or are there are any rough corners still left before ABIv2 and unstable can split in a feature freeze? Repository: rC Clang https://reviews.llvm.org/D52660 _

[PATCH] D52660: [CMake][Fuchsia] Use libc++ ABIv2 for Fuchsia toolchain

kristina added a comment. My misunderstanding then, I was eventually hoping for a feature freeze of ABIv2 to a point where it's considered stable so any alterations to things like internal layout of std::string (ie. like SBO that came with ABIv2) happen in `unstable/std::__3` as this allows for

[PATCH] D52673: [HIP] Remove disabled irif library

kristina added a comment. If you don't mind, can you please do another revision with full context (`-U9`), makes it easier to review and makes it less likely that `svn patch` screws up. Repository: rC Clang https://reviews.llvm.org/D52673 _

[PATCH] D50403: [clang-format]AlignConsecutiveAssignments

kristina added subscribers: djasper, klimek, kristina. kristina added a comment. Please upload your diffs with full context (use `-U9` when doing the diff) and update them. I don't think there is a code owner for this part but @klimek and @djasper do most reviews to do with clang-format, so

[PATCH] D52705: First-pass at ARM hard/soft-float multilib driver (NOT WORKING YET)

kristina added subscribers: rsmith, kristina. kristina added reviewers: t.p.northover, rsmith. kristina added a comment. Accepted blocking differential https://reviews.llvm.org/D52149, updated reviewers to include ARM code owner and @rsmith. Repository: rC Clang https://reviews.llvm.org/D527

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

kristina added a reviewer: kristina. kristina added a comment. Able to reproduce easily: clang-8: /SourceCache/llvm-trunk-8.0/include/llvm/ADT/StringRef.h:143: char llvm::StringRef::front() const: Assertion `!empty()' failed. Will try it with patches applied in a moment. Repository: rC Cl

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

kristina added inline comments. Comment at: lib/Lex/PPDirectives.cpp:1892 if (!File) { -while (!isAlphanumeric(Filename.front())) { +while (!Filename.empty() && !isAlphanumeric(Filename.front())) { Filename = Filename.drop_front(); --

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

kristina added inline comments. Comment at: lib/Lex/PPDirectives.cpp:1892 if (!File) { -while (!isAlphanumeric(Filename.front())) { +while (!Filename.empty() && !isAlphanumeric(Filename.front())) { Filename = Filename.drop_front(); --

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

kristina added a comment. Have you tested that build is successful and obviously the FileCheck test should pass now? What sort of behavior does this produce when you attempt to use an invalid path like that now when compiling with clang? Just a predecessor diagnostic? I'm updating software on

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

kristina added inline comments. Comment at: lib/Lex/PPDirectives.cpp:1898 + } + if (Filename.empty()) +return Filename; sammccall wrote: > simplify the logic by merging with the while loop? (and drop the assert) I thought the assert

[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

[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] 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 http://lists.llvm.org/c

[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 `format::getLLVMStyle(format::FormatStyle::LK_TableGe

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

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

[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 https://reviews.llvm

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

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

[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/ https://reviews.ll

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

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/ https://re

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

<    1   2