Page MenuHomePhabricator

kristina (Kristina Brooks)
Bikeshed Expert

Projects

User does not belong to any projects.

User Details

User Since
Apr 8 2018, 2:18 PM (49 w, 4 d)

Maintainer of an LLVM backend fork for Broadcom VideoCore4, never went upstream as Julian's GCC port turned out to work a lot better and did not require a separate assembler/linker. Expert on Darwin kernel, Mach-O file format, dynamic linking infrastructure on Darwin (dyld, shared caches, codesigning).

Personal GitHub Account: https://github.com/christinaa
Personal Blog: http://crna.cc/
Use notstina (at) gmail (dot) com if you need to email me.

Not representing any organization.

Recent Activity

Tue, Mar 12

kristina committed rG14179673e27b: [Docs] Add note about legacy PM to Ch4 of tutorial (authored by kristina).
[Docs] Add note about legacy PM to Ch4 of tutorial
Tue, Mar 12, 8:45 AM
kristina committed rL355930: [Docs] Add note about legacy PM to Ch4 of tutorial.
[Docs] Add note about legacy PM to Ch4 of tutorial
Tue, Mar 12, 8:45 AM
kristina closed D59258: [Docs] Add a note about legacy FunctionPassManager to the LLVM tutorial..
Tue, Mar 12, 8:45 AM · Restricted Project
kristina added a comment to D59258: [Docs] Add a note about legacy FunctionPassManager to the LLVM tutorial..

I did, there are no references to any PassManager related material in the first three chapters.

Tue, Mar 12, 8:37 AM · Restricted Project
kristina created D59258: [Docs] Add a note about legacy FunctionPassManager to the LLVM tutorial..
Tue, Mar 12, 8:31 AM · Restricted Project
kristina added a comment to D59243: [format] \t => ' '.

Please make sure to specify Differential Revision: https://reviews.llvm.org/D59243 with Dxxxxx being the differential number in the URL when committing (on a separate line in the commit message) which will autmatically close the differential and link it to the commit which makes sure there are no stray/stale differentials. Alternatively manually link the diff to the commit (you can do that by simply quoting the commit as rL<SVN Rev> and then closing the differential manually.

Tue, Mar 12, 3:03 AM · Restricted Project
kristina committed rG5b1e1c0537d0: Very minor typo. NFC (authored by kristina).
Very minor typo. NFC
Tue, Mar 12, 12:11 AM
kristina committed rL355895: Very minor typo. NFC.
Very minor typo. NFC
Tue, Mar 12, 12:07 AM
kristina closed D59241: [typo] we we => we.
Tue, Mar 12, 12:07 AM · Restricted Project

Mon, Mar 11

kristina accepted D59241: [typo] we we => we.

we were?

Mon, Mar 11, 11:57 PM · Restricted Project

Sun, Mar 3

kristina committed rG24659eb2e766: Remove large amount of empty lines mid-file. NFC (authored by kristina).
Remove large amount of empty lines mid-file. NFC
Sun, Mar 3, 5:22 AM
kristina committed rL355286: Remove large amount of empty lines mid-file. NFC.
Remove large amount of empty lines mid-file. NFC
Sun, Mar 3, 5:22 AM

Thu, Feb 28

kristina added a comment to D17741: adds __FILE_BASENAME__ builtin macro.

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)?

Thu, Feb 28, 9:12 AM

Wed, Feb 27

Herald added a project to D56383: [XRay][tools] Use symbols instead of function id: Restricted Project.

Gentle ping for author, has this landed or still in the backlog? (If it's landed can you attach the commit and close it, please? Just trying to to avoid diffs on Phab going stale.)

Wed, Feb 27, 4:35 AM · Restricted Project
kristina added a comment to D57400: Add a .gitignore file to the root that ignores any files outside of the project directories..

What about using /*/ at the ignore pattern? This allows top-level files, and makes only new top-level *directories* require an ignore update. To my mind, that seems a bit more narrowly scoped and might be a bit less surprising. Thoughts?

Wed, Feb 27, 4:33 AM

Tue, Feb 26

kristina committed rG76eb4b02d93b: Update docs of memcpy/move/set wrt. align and len (authored by kristina).
Update docs of memcpy/move/set wrt. align and len
Tue, Feb 26, 10:55 AM
kristina committed rL354911: Update docs of memcpy/move/set wrt. align and len.
Update docs of memcpy/move/set wrt. align and len
Tue, Feb 26, 10:52 AM
kristina closed D57600: update docs of memcpy/memmove/memset re: alignment and len=0.
Tue, Feb 26, 10:52 AM · Restricted Project

Mon, Feb 25

kristina added a comment to D57600: update docs of memcpy/memmove/memset re: alignment and len=0.

@RalfJung Do you need someone to commit this? I can do it if you'd like.

Mon, Feb 25, 1:36 PM · Restricted Project
kristina accepted D57600: update docs of memcpy/memmove/memset re: alignment and len=0.

Please make sure you have llvm-commits added as a subscriber when creating patches in the future (I'm not sure why Herald sometimes doesn't do it). The patch that changed this was rL322965, and I fixed one of the related regressions, and was meaning to update the docs but never did, so LGTM and I'm happy signing off on this.

Mon, Feb 25, 9:31 AM · Restricted Project

Sun, Feb 24

kristina committed rG103799c06028: Fix accidentally used hard tabs. NFC (authored by kristina).
Fix accidentally used hard tabs. NFC
Sun, Feb 24, 10:09 AM
kristina committed rC354752: Fix accidentally used hard tabs. NFC.
Fix accidentally used hard tabs. NFC
Sun, Feb 24, 10:06 AM
kristina committed rL354752: Fix accidentally used hard tabs. NFC.
Fix accidentally used hard tabs. NFC
Sun, Feb 24, 10:06 AM
kristina committed rC354751: Wrap code for builtin_assume_aligned at 80 col.NFC.
Wrap code for builtin_assume_aligned at 80 col.NFC
Sun, Feb 24, 9:58 AM
kristina committed rG716cbfb4640f: Wrap code for builtin_assume_aligned at 80 col.NFC (authored by kristina).
Wrap code for builtin_assume_aligned at 80 col.NFC
Sun, Feb 24, 9:57 AM
kristina committed rL354751: Wrap code for builtin_assume_aligned at 80 col.NFC.
Wrap code for builtin_assume_aligned at 80 col.NFC
Sun, Feb 24, 9:57 AM

Feb 16 2019

kristina added a comment to D57400: Add a .gitignore file to the root that ignores any files outside of the project directories..

I still don't like it...It's different, unusual, and IMO surprising to have such a wildcard ignore.

We haven't tended to have lots of random accidental file additions before, and while someone may surely mess up again, I don't think it likely to be a common occurrence.

I'd much prefer simply the targeted ignore of "/build*", at least for starters.

Feb 16 2019, 6:46 PM
kristina committed rG440f8f0c2b45: [LLVMSupport]: Remove a severely outdated README. (authored by kristina).
[LLVMSupport]: Remove a severely outdated README.
Feb 16 2019, 5:52 PM
kristina committed rL354209: [LLVMSupport]: Remove a severely outdated README..
[LLVMSupport]: Remove a severely outdated README.
Feb 16 2019, 5:52 PM
kristina planned changes to D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator..
Feb 16 2019, 3:20 PM · Restricted Project
kristina added a comment to D57400: Add a .gitignore file to the root that ignores any files outside of the project directories..

I'm in favor of this and I agree with @pcc's points and reasoning behind this.

Feb 16 2019, 3:14 PM

Feb 15 2019

kristina added a comment to D57429: [docs] Document ignoring build directory.

I actually like the D57400 approach more, it's much easier to keep a list of known top level projects as a whitelist. There are many names people could give to such a build directory (out or build or b) not to mention any other cruft one may have there. (ie. I build a libc as part of toolchain build).

Feb 15 2019, 2:37 AM · Restricted Project

Feb 6 2019

kristina added a comment to D56943: [clang-format][NFC] Allow getLLVMStyle() to take a language.

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_TableGen)? I'm not entirely sure how useful this particular change is given that there's no linked patches related to your use case, I think adding those would help as well (possibly as a separate dependent patchset).

Feb 6 2019, 7:46 PM · Restricted Project, Restricted Project

Jan 29 2019

kristina added a comment to D57128: Add --unwindlib=[libgcc|compiler-rt] to parallel --rtlib=.

FWIW, this also broke my bootstraps of mingw-w64 environments/toolchains. After building compiler-rt builtins, before having any libunwind/libcxx built, I previously regarded my toolchain as complete for building and testing C apps and libraries, but that fails now.

Would it be possible to add a third alternative, --unwindlib=none, to signal that while I'm using --rtlib=compiler-rt, I don't want to link to any unwinder? (In my case, I'm injecting libunwind in libc++.a so it only gets added when linking C++ code.) Or at least make it possible to only add this linker flag when linking C++? Alternatively I'll need to provide a dummy libunwind.a until the real one has been built.

Jan 29 2019, 12:27 AM

Jan 28 2019

kristina added a comment to D57128: Add --unwindlib=[libgcc|compiler-rt] to parallel --rtlib=.

One more thing, shouldn't libunwind be only included for C++ (since it's the internal dependency of libc++abi)? There shouldn't be any need to include it for C.

Jan 28 2019, 9:06 PM
kristina accepted D55439: [MC] Do not consider .ifdef/.ifndef as a use.

LGTM.

Jan 28 2019, 8:38 AM

Jan 20 2019

kristina committed rL351722: [llgo]: fix compilation under current llvm.
[llgo]: fix compilation under current llvm
Jan 20 2019, 8:37 PM
kristina closed D56638: [llgo]: Somewhat revive it and make it buildable with current trunk..
Jan 20 2019, 8:37 PM
kristina added a comment to D56638: [llgo]: Somewhat revive it and make it buildable with current trunk..

Revised, landing it without split stack stuff, should autoupdate the revision here.

Jan 20 2019, 8:32 PM

Jan 19 2019

kristina committed rL351639: Remove a period from CREDITS.TXT (testing email change). NFC.
Remove a period from CREDITS.TXT (testing email change). NFC
Jan 19 2019, 1:15 AM

Jan 18 2019

kristina added a comment to D56638: [llgo]: Somewhat revive it and make it buildable with current trunk..

Ping.

Jan 18 2019, 1:58 PM

Jan 17 2019

kristina added a reviewer for D55439: [MC] Do not consider .ifdef/.ifndef as a use: kristina.

LGTM if GAS does allow multiple uses of .set to change the value. One nit on the test, can you add a case where you don't have an .if directive like:

Jan 17 2019, 8:10 AM

Jan 14 2019

kristina committed rL351082: [Sema] Expose a control flag for integer to pointer ext warning.
[Sema] Expose a control flag for integer to pointer ext warning
Jan 14 2019, 10:21 AM
kristina committed rC351082: [Sema] Expose a control flag for integer to pointer ext warning.
[Sema] Expose a control flag for integer to pointer ext warning
Jan 14 2019, 10:20 AM
kristina closed D56241: [Sema] expose a control flag for integer to pointer ext warning.
Jan 14 2019, 10:20 AM · Restricted Project
kristina added a comment to D56241: [Sema] expose a control flag for integer to pointer ext warning.

Going to test and land it as requested by @lebedev.ri on IRC.

Jan 14 2019, 9:26 AM · Restricted Project

Jan 13 2019

kristina added a comment to D56638: [llgo]: Somewhat revive it and make it buildable with current trunk..

Currently actual tests would require using libgcc, as compiler-rt does not support split stack yet. If libgcc is required for tests then I don't think I can accurately make one. So if you require a test for this, it's likely that I'll have to mark this as "Changes Planned" for now until compiler-rt is sufficient to link this, unfortunately. Up to you really, though in my opinion even without tests this is better than it failing to compile. And the reasons for avoiding libgcc are fairly complicated, my intentions currently are to stay as far away from GNU runtime components as possible.

Jan 13 2019, 2:40 PM
kristina added a comment to D28791: [compiler-rt][crt] Simple crtbegin and crtend implementation.

FWIW, I completely support landing this. I think the opposition is essentially "it would be better to not design a libc in that way" which is certainly a valid opinion, but given the existence of such libc designs widely deployed and widely used, it seems very good to lay groundwork for LLVM to support them.

I also think we should *not* be pointing at GNU or any other project here, and instead simply state that these exist to support using LLVM in conjunction with systems whose libc chooses this particular design pattern.

I don't think LLVM should be in the business of trying to cast some kind of "you should feel bad" judgement on such systems. If someone in the LLVM community wants to develop a libc as part of LLVM (which I would quite like to see), then that is the place to debate libc design decisions, and not anywhere else.

So, that said, does anyone have comments on the code itself? If no one is up to commenting on the code and giving Petr direct feedback there, I'll just LGTM this because I think it is long (looooooong) overdue.

Lastly, to address specific questions here:

That's fine, it's just not the state of the world and we should support compiling appropriately.

I don't want to pick a fight, but there can be used exactly same argument to defend pulling libgcc here.

That doesn't make sense to me -- the LLVM project has lots of reasons why it doesn't pull GPL-ed code into its tree.

But yes, we are going to be in the business of being 100% and fully compatible with libgcc in order to effectively support platforms based on libcs that follow the design pattern of glibc, certainly including platforms that use glibc. Is that work? Yes. Should the project do it? Absolutely yes. Just like we work hard to be compatible with the gcc commandline and other layers of compatibility, we should work to support these platforms well.

Jan 13 2019, 1:34 AM · Restricted Project, Restricted Project

Jan 12 2019

kristina added a comment to D56638: [llgo]: Somewhat revive it and make it buildable with current trunk..

Changes to runtime.go address the regression caused by rL322965.

Jan 12 2019, 8:22 AM
kristina created D56638: [llgo]: Somewhat revive it and make it buildable with current trunk..
Jan 12 2019, 8:18 AM

Jan 10 2019

kristina added a comment to D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator..

Addressed all comments, will wait for feedback and then do the final draft, and if people are happy with that, I'll lint the whole file and then commit it.

Jan 10 2019, 2:41 AM · Restricted Project

Jan 9 2019

kristina added a comment to D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator..

! In D56482#1350874, @kristina wrote:

I only added a section, all the warnings are from existing bits below. Not sure if it's a good idea to reformat it as part of the diff as it will make it harder to see the added section. Can do it afterwards but that's what's currently being used to generate documentation.

Sorry I wasn't clear, I'm using a script from D55523: [clang-tidy] Linting .rst documentation to validate .rst files, if you read the revision there is some info on the rational. (I simply ran it here on a copy of the file you are editing prior to your revision). You'll see from D55523 that I mention that I saw a lot of review comments which amounted to stylistic changes in the .rst files, and yet I see a lot of reviewers time pointing these out, time and time again and its all simple stuff that could be automated. (or at least added to such a how to guide)

it would be good if we could ensure .rst files are checked prior to review (how we do this I don't know) without initially generating 10,000 of minor whitespaces changes.

when I reviewed all .rst files currently checked into LLVM I found 10,000's of simple violations (maybe people aren't worried about it but certainly it comes up a lot in reviews and causes the second or third revisions)

I think another point is that lot of review comments could potentially be caught by clang-tidy, there is sometimes lots of the review conversations especially with new contributors, about "don't use anonymous namspaces but prefer static functions","clang-format the file", don't overly use auto", don't have "const value types" all stuff that is in the CodingStyle but is easily missed when making or updating a review.

It would be good if we had a mechanism to be able to validate a patch prior to commit, maybe using something like clang-tidy-diff.py in combination with something like the .rst linter to reduce the iterations on a revision, speed up development and reduce the burden that seems present on a group of dedicated but possible select people.

Jan 9 2019, 12:52 PM · Restricted Project
kristina added a comment to D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator..

$ ./validate_check.py --rst ../../../../../docs/Phabricator.rst
Checking ..\..\..\..\..\docs\Phabricator.rst...
warning: line 97 multiple blank lines
warning: line 166 is in excess of 80 characters (82) : as it will retrieve reviewers, the `Differential Revision`, etc from the review
...
warning: line 175 contains double spaces
warning: line 188 is in excess of 80 characters (97) : The first command will take the latest version of the reviewed patch and apply it to the working
...
warning: line 194 is in excess of 80 characters (111) : This presumes that the git repository has been configured as described in :ref:developers-work-with-git-svn.
...
warning: line 203 multiple blank lines
warning: line 205 is in excess of 80 characters (84) : current `master and will create a commit corresponding to D<Revision>` with a
...
warning: line 208 is in excess of 80 characters (85) : Check you are happy with the commit message and amend it if necessary. Now switch to
...
warning: line 218 multiple blank lines
warning: line 219 multiple blank lines

Jan 9 2019, 2:45 AM · Restricted Project
kristina added a reviewer for D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator.: lebedev.ri.
Jan 9 2019, 1:59 AM · Restricted Project
kristina added inline comments to D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator..
Jan 9 2019, 1:59 AM · Restricted Project
kristina updated the diff for D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator..

Spellcheck.

Jan 9 2019, 1:54 AM · Restricted Project
kristina created D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator..
Jan 9 2019, 1:45 AM · Restricted Project

Jan 8 2019

kristina updated subscribers of rL350671: [PGO] Use SourceFileName rather module name in PGOFuncName.

+llvm-commits

Jan 8 2019, 5:25 PM
kristina raised a concern with rL350671: [PGO] Use SourceFileName rather module name in PGOFuncName.

Unaddressed issues in pre-commit review.

Jan 8 2019, 5:05 PM
kristina added a comment to D55769: Add support for demangling msvc's noexcept types to llvm-undname.

lgtm. I'm technically on vacation so rnk@ might be able to commit it sooner than I can, but if he doesn't, then I'll commit it when I find some spare cycles. Thanks for fixing this!

Hi @zturner, can you please commit this when you have a chance?

Jan 8 2019, 11:40 AM
kristina added a comment to D56327: [PGO] Use SourceFileName rather module name in PGOFuncName.

Differential Review is mostly for pre-commit review, add in "Differential Revision: https://reviews.llvm.org/DXXXXX" to the commit message and it will automatically link the commit and diff and close it. As an alternative you can do that manually, It seems bigger patch has already been committed in rL350579 (without review) and this was pending review rL350442. I've linked those in but if you say it's been fixed in the committed version and this is a followup, can you rebase the patch on the current trunk? For post-commit reviews Audit may be a better place if you need that.

Jan 8 2019, 11:07 AM
kristina added 1 commit(s) for D56327: [PGO] Use SourceFileName rather module name in PGOFuncName: rL350442: [PGO] Use SourceFileName rather module name in PGOFuncName.
Jan 8 2019, 11:03 AM
kristina added an edge to rL350442: [PGO] Use SourceFileName rather module name in PGOFuncName: D56327: [PGO] Use SourceFileName rather module name in PGOFuncName.
Jan 8 2019, 11:03 AM
kristina added an edge to rL350579: [PGO] Use SourceFileName rather module name in PGOFuncName: D56327: [PGO] Use SourceFileName rather module name in PGOFuncName.
Jan 8 2019, 10:57 AM
kristina added 1 commit(s) for D56327: [PGO] Use SourceFileName rather module name in PGOFuncName: rL350579: [PGO] Use SourceFileName rather module name in PGOFuncName.
Jan 8 2019, 10:57 AM

Jan 7 2019

kristina added a comment to D56383: [XRay][tools] Use symbols instead of function id.

Quite a big style nit. I'd say get rid of the switch completely.

Jan 7 2019, 9:41 PM · Restricted Project
kristina added a comment to D17741: adds __FILE_BASENAME__ builtin macro.

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 probably be included here) but I would appreciate some form of consensus since this exact idea with a special macro for filename was previously rejected.

Jan 7 2019, 9:29 PM
kristina added a reviewer for D56383: [XRay][tools] Use symbols instead of function id: kristina.

Added some inline comments.

Jan 7 2019, 9:09 PM · Restricted Project
kristina requested changes to D56327: [PGO] Use SourceFileName rather module name in PGOFuncName.

I feel like this needs a test or amending an existing test, a bit too tired right now to patch this into my fork and run the test suite, but I feel as-is, this could cause test failures (and if it doesn't, that's likely indicative of missing coverage). Please address that before committing it. Also, see inline comments, first one is a style nit but I'm more concerned about uint32_t StripLevel = StaticFuncFullModulePrefix ? 0 : -1;.

Jan 7 2019, 8:53 PM
kristina added a comment to D47073: Document and Enforce new Host Compiler Policy.

I could see it making bootstrapping LLVM a lot harder (plus "golden" versions of compilers tend to be older than three years). I like the spirit of this policy, but the timeframe is pretty narrow, does GCC currently have a "golden" version? I think we should at least make sure "golden" compilers are supported for bootstrap.

Jan 7 2019, 7:18 PM
kristina added a reviewer for D56415: NFC: Port QueryParser to StringRef: kristina.

LGTM aside from one comment.

Jan 7 2019, 6:05 PM
kristina added a comment to D56419: [gn build] Move .gn file to the root of the monorepo.

I'm neutral on the issue but I would urge Nico to really think this through, lashback from some major downstream Linux distro vendors last time was pretty nasty, including flat out dropping Clang/LLVM support if CMake became a second class citizen. Is rehashing the same thing again worth it? Consider that a lot of people, no matter what you say will see it as GN slowly creeping in to replace CMake, not many are familiar with it outside Google or Chromium/Skia contributors.

I'd like to reiterate that this patch does not change anything about LLVM's build system situation. CMake is the supported build system. GN is 100% unsupported (as I've added as a comment to the file as I moved it over). I do not intend to push for changing this.

Jan 7 2019, 5:46 PM
kristina added a comment to D56419: [gn build] Move .gn file to the root of the monorepo.

I'm neutral on the issue but I would urge Nico to really think this through, lashback from some major downstream Linux distro vendors last time was pretty nasty, including flat out dropping Clang/LLVM support if CMake became a second class citizen. Is rehashing the same thing again worth it? Consider that a lot of people, no matter what you say will see it as GN slowly creeping in to replace CMake, not many are familiar with it outside Google or Chromium/Skia contributors.

Jan 7 2019, 5:01 PM

Jan 3 2019

kristina added a comment to D17741: adds __FILE_BASENAME__ builtin macro.

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 value. The common way of getting the short filename in code would the the following (the #else case) which is a constant evaluated but has a side effect of dumping the full paths in rodata regardless (ICF seems unable to clean them up even in full LTO builds).

Jan 3 2019, 11:17 AM
kristina committed rL350342: [MCStreamer] Use report_fatal_error in EmitRawTextImpl.
[MCStreamer] Use report_fatal_error in EmitRawTextImpl
Jan 3 2019, 10:46 AM
kristina closed D56245: Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC..
Jan 3 2019, 10:46 AM
kristina updated the diff for D56245: Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC..

Revised to be a fatal error after discussion and added comment for any future maintainers. Also made empty braced blocks consistent.

Jan 3 2019, 10:40 AM
kristina added a comment to D17741: adds __FILE_BASENAME__ builtin macro.

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 this to get into clang: with 1.2 million lines of code, the regular placement of log statements and custom asserts leads to megabytes in binary size from all the FILE usages, and that could easily be a few hundred KB with this kind of support in clang.

Jan 3 2019, 8:04 AM
kristina added a reviewer for D56245: Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC.: rnk.
Jan 3 2019, 6:58 AM
kristina added a comment to D17741: adds __FILE_BASENAME__ builtin macro.

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 weird extension to do meta-programming using the preprocessor, that's the implementation with no extra parameters (just as an idea, I know the implementation is not great):

Jan 3 2019, 6:53 AM

Jan 2 2019

kristina added a comment to D56206: Introduce `RemoteAddressSpaceView` and `VMReadContext`..

Would it make sense to also extend this to Linux using process_vm_readv/process_vm_writev?

Jan 2 2019, 11:14 PM
kristina created D56245: Use llvm_unreachable instead of errs+abort in MCStreamer.cpp (in EmitRawTextImpl). NFC..
Jan 2 2019, 10:34 PM
kristina committed rL350286: Don't go over 80 chars in MCStreamer.cpp. NFC..
Don't go over 80 chars in MCStreamer.cpp. NFC.
Jan 2 2019, 10:10 PM

Dec 30 2018

kristina added a comment to D55953: Android is not GNU, so don't claim that it is..

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 making sure __gnu_linux__ being defined explicitly excludes Android from those paths.

Dec 30 2018, 3:07 AM

Dec 15 2018

kristina added a comment to D54277: Move RedirectingFileSystem interface into header..

I'm not sure if this is way out of scope for this patch, but if I understand correctly this could be used for extending it to, for example, handle things like Perforce-style paths which could indeed be useful, although I'm not sure if that is possible (ie. would //depot/asdf behave differently on Windows and Linux/Darwin/any other OS that uses Unix-style paths?). If this is with the intention to allow extending RedirectingFileSystem downstream then I think it may be worth splitting it up from the YAML based implementation altogether, allowing custom path handling where needed (as a separate downstream extension), with the concrete reference implementation of RedirectingFileSystem (the YAML based one) being a separate thing. This would make it more extensible from the POV of downstream forks that may desire to implement path redirection that is too complicated to do using YAML mappings, but still fundamentally rely on some sort of path remapping.

Dec 15 2018, 1:11 PM
kristina added a comment to D55734: [analyzer] Revise GenericTaintChecker's internal representation.

If you don't mind, please submit diffs with full context (diff -U99999 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 statement ifs should have the statements within them aligned without adding another indentation level for every line break within the statement, also in case below dyn_cast_or_null may eliminate a need for an additional check in which case you can just use && instead.

Dec 15 2018, 2:23 AM
kristina added a comment to D54864: Introduce llvm-objdump man page.

Ping for author, this has been approved for a while, do you need this committed?

Dec 15 2018, 1:52 AM

Dec 6 2018

kristina accepted D54864: Introduce llvm-objdump man page.

LGTM.

Dec 6 2018, 1:24 PM
kristina requested changes to D55150: Emit warnings from the driver for use of -mllvm or -Xclang options..

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, and then users adding additional options somewhere else in the build system will miss the warning.

Instead, it would probably be better to rename Xclang and mllvm to something that makes it clear the user is doing something unsupported. Maybe "--unstable-llvm-option" and "--unstable-clang-option" or something like that. (This will lead to some breakage, but the breakage is roughly equivalent for anyone using -Werror.)

Dec 6 2018, 11:57 AM
kristina added a comment to D55150: Emit warnings from the driver for use of -mllvm or -Xclang options..

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, they told me "but if I'm not supposed to use them, why are they advertised in --help"?

thakis@thakis:~/src/llvm-build$ bin/clang --help | grep Xclang
  -Xclang <arg>           Pass <arg> to the clang compiler
thakis@thakis:~/src/llvm-build$ bin/clang --help | grep mllvm
  -mllvm <value>          Additional arguments to forward to LLVM's option processing

Which to me is a valid point! Maybe we should remove them from --help or say "internal only, don't usually use" there?

Dec 6 2018, 11:12 AM
kristina added a comment to D55150: Emit warnings from the driver for use of -mllvm or -Xclang options..

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 command-line. But, neither -Xclang nor -mllvm obviously scream out "don't use this!", and so people may then add them to their buildsystems without causing reviewers to say "Wait...really? Are you sure that's a good idea?".

Dec 6 2018, 10:35 AM
kristina resigned from D50858: [M680x0] Add ELF and Triple info.
Dec 6 2018, 12:51 AM
kristina added a comment to D55150: Emit warnings from the driver for use of -mllvm or -Xclang options..

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 <llvm_tool> --help output requiring explicitly specifying that hidden flags be shown. One real use case being a dozen of patches to my downstream LLVM/Clang fork, for example *very* experimental support for SEH on Itanium ABI. I feel like this is going to affect developers more than users as now additional flags will have to be passed to suppress the warnings generated from using flags to debug/diagnose passes by code authors themselves. I feel like using -mllvm already implies explicit understanding of that, and of the cl::opt semantics/purpose as well as the flags being generally out of public view unless one gets them from full help (which explicitly shows all registered flags, hidden or not), or from source code which is most likely to be the developers themselves.

Dec 6 2018, 12:26 AM

Dec 5 2018

kristina committed rC348368: [Haiku] Support __float128 for x86 and x86_64.
[Haiku] Support __float128 for x86 and x86_64
Dec 5 2018, 7:08 AM
kristina committed rL348368: [Haiku] Support __float128 for x86 and x86_64.
[Haiku] Support __float128 for x86 and x86_64
Dec 5 2018, 7:08 AM
kristina closed D54901: [Haiku] Support __float128 for Haiku x86 and x86_64.
Dec 5 2018, 7:08 AM · Restricted Project

Nov 30 2018

kristina added a comment to D55045: Add a version of std::function that includes a few optimizations..

FWIW, I'm slightly in favor of using different headers. For example, I just realized that this change currently leaves a bunch of baggage symbols laying around that the new code won't use, it would be easier to see that kind of thing if the internals of each function were just in it's own file.

I'm strongly against it. The harder it is to see all the implementations of a given function f, the harder it is to know they exist and keep them in sync. In my experience spreading these things out leads to real bugs.

@ldionne wrote:

This is why I would like for us to plan on a solution that is more easily maintainable in the face of N implementations, not just 2 implementations (disregarding the C++03 implementations).

I would like that too. but that's not a blocking issue on this review.
I also don't agree that we'll see more std::function implementations in the future. We won't. We're not going to standardize additional features to std::function unless they're not ABI breaking. That is, if the standard does function it'll won't require another version of it.
Maybe you were thinking about things like std::unique_function?

Note that having the C++03 functions be in a separate file with no relation or interleaving with the C++11 implementation has caused plenty of bugs in the past where they diverge.

Nov 30 2018, 9:22 PM
kristina added a reviewer for D54277: Move RedirectingFileSystem interface into header.: kristina.
Nov 30 2018, 6:16 AM

Nov 29 2018

kristina updated subscribers of D55045: Add a version of std::function that includes a few optimizations..

Subject: Re: [PATCH] D55045: Add a version of std::function that includes a few optimizations.
From: Jordan Soyke <jsoyke@google.com>

Do we need an additional config option for this?

On Thu, Nov 29, 2018 at 07:56 Kristina Brooks via Phabricator <reviews@reviews.llvm.org> wrote:

kristina added reviewers: ldionne, EricWF, kristina, phosek.
kristina added a comment.

Added reviewers. This will be a break in ABIv2 and Unstable ABI (since
they're the same at the moment), only Fuchsia uses it at the moment, as far
as I'm aware.

Nov 29 2018, 6:16 AM
kristina added reviewers for D55045: Add a version of std::function that includes a few optimizations.: ldionne, EricWF, kristina, phosek.

Added reviewers. This will be a break in ABIv2 and Unstable ABI (since they're the same at the moment), only Fuchsia uses it at the moment, as far as I'm aware.

Nov 29 2018, 4:56 AM