kristina (Kristina Brooks)
Bikeshed Expert

Projects

User Details

User Since
Apr 8 2018, 2:18 PM (32 w, 1 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/

Not representing any organization.

Recent Activity

Yesterday

kristina added a comment to D54379: Add Hurd toolchain support to Clang.

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 a lack of negative tests (ie. when invalid input is supplied and matches the triple suffix). Feel free to run tests though, may find something before me, I'm a bit too tired to reconfigure my buildbot to do what I want right now, so I'll leave it until when I'm up. So yeah I may be being a bit nitpicky but I think tests could cover a little bit more.

Sun, Nov 18, 12:55 AM

Fri, Nov 16

kristina added a comment to D54379: Add Hurd toolchain support to Clang.

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 just a suggestion. Once you confirmed that the triple in question is the one you're looking for (from the suffix), you can alter the existing triple. The std::string should be avoided here since even a zero length SmallVector will guarantee not allocating anything unless used. This may be worth factoring out into a separate function entirely, and another important point is avoiding any sort of unneeded overhead unless you've confirmed that it's the Hurd triple.

Fri, Nov 16, 7:48 PM
kristina added a reviewer for D54379: Add Hurd toolchain support to Clang: rjmccall.
Fri, Nov 16, 7:34 PM
kristina added a comment to D54379: Add Hurd toolchain support to Clang.

Also, this needs unit tests and FileCheck tests.

Fri, Nov 16, 2:12 AM
kristina added inline comments to D54379: Add Hurd toolchain support to Clang.
Fri, Nov 16, 2:00 AM
kristina added inline comments to D54379: Add Hurd toolchain support to Clang.
Fri, Nov 16, 1:41 AM
kristina added a comment to D54379: Add Hurd toolchain support to Clang.

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 instead, that would also save you from needing to resort to using strlen later (sorry for previous comment, I didn't mean strcpy).

Fri, Nov 16, 1:28 AM
kristina added a comment to D54379: Add Hurd toolchain support to Clang.

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.

Fri, Nov 16, 1:15 AM

Thu, Nov 15

kristina abandoned D54605: [lld][ELF] Add negative test coverage for unknown "-z" flags..

Ah alright no problem, seems I didn't notice. Thanks for the quick review.

Thu, Nov 15, 5:25 PM
kristina updated the diff for D54605: [lld][ELF] Add negative test coverage for unknown "-z" flags..

Revised to drop the change.

Thu, Nov 15, 5:04 PM
kristina added a comment to D54605: [lld][ELF] Add negative test coverage for unknown "-z" flags..

Shall I fully abandon the revision or want me to leave the test for unknown flag failure in? (Unless one already exists and I missed it, additional coverage can't hurt regardless)

Thu, Nov 15, 4:51 PM
kristina added a comment to D54379: Add Hurd toolchain support to Clang.

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 D54378).

Thu, Nov 15, 4:49 PM
kristina requested changes to D54379: Add Hurd toolchain support to Clang.
Thu, Nov 15, 4:40 PM
kristina added inline comments to D54379: Add Hurd toolchain support to Clang.
Thu, Nov 15, 4:40 PM
kristina added a comment to D54379: Add Hurd toolchain support to Clang.

Added first batch of comments regarding the patch. Some style, some semantics-related.

Thu, Nov 15, 4:39 PM
kristina added reviewers for D54379: Add Hurd toolchain support to Clang: aaron.ballman, erik.pilkington, rsmith.

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 Clang CR than me and who can hopefully weigh in some opinions. (Sorry about the delays, looks like I'll be the one seeing addition of this target through, not landed the LLVMSupport change yet since I want to make sure this set of patches is good, otherwise the LLVMSupport change on its own holds little weight)

Thu, Nov 15, 4:12 PM
kristina added a comment to D54605: [lld][ELF] Add negative test coverage for unknown "-z" flags..

No regressions in ELF test suite and test runs just fine:

Thu, Nov 15, 3:53 PM
kristina updated the diff for D54605: [lld][ELF] Add negative test coverage for unknown "-z" flags..

Revised, newline at the end of the test file.

Thu, Nov 15, 3:46 PM
kristina created D54605: [lld][ELF] Add negative test coverage for unknown "-z" flags..
Thu, Nov 15, 3:44 PM

Tue, Nov 13

kristina added an edge to rL346763: [libcxx] GNU/Hurd uses BSD-based interfaces, but does not (and won't) provide…: D54338: Fix threads build on GNU/Hurd.
Tue, Nov 13, 11:50 PM
kristina added 1 commit(s) for D54338: Fix threads build on GNU/Hurd: rL346763: [libcxx] GNU/Hurd uses BSD-based interfaces, but does not (and won't) provide….
Tue, Nov 13, 11:50 PM

Sun, Nov 11

kristina added a comment to D54338: Fix threads build on GNU/Hurd.

Doesn't __APPLE__ imply __MACH__ anyway outside of targets that are way out of scope of libc++? Since IIRC libc++ only runs on Darwin and only in usermode, which has the __APPLE__ predefined macro even for stuff like PureDarwin .

Sun, Nov 11, 9:04 PM
kristina retitled D54379: Add Hurd toolchain support to Clang from Add Hurd support to Add Hurd toolchain support to Clang.
Sun, Nov 11, 6:24 PM
kristina retitled D54378: Add Hurd triplet to LLVMSupport from Add Hurd triplet to Add Hurd triplet to LLVMSupport.
Sun, Nov 11, 6:21 PM
kristina accepted D54378: Add Hurd triplet to LLVMSupport.

Alright, as far as this part goes everything LGTM.

Sun, Nov 11, 6:19 PM
kristina added a comment to D54380: [llvm-objdump] Add symbol 'O' for object data.

Grouped and linked all related commits. Closing.

Sorry for that, I should have run 'test all' locally ... Sorry for bothering you too much! Thanks a lot!

Sun, Nov 11, 5:27 PM
kristina committed rC346628: [CodeGen][CXX]: Fix no_destroy CG bug under specific circumstances.
[CodeGen][CXX]: Fix no_destroy CG bug under specific circumstances
Sun, Nov 11, 5:23 PM
kristina committed rL346628: [CodeGen][CXX]: Fix no_destroy CG bug under specific circumstances.
[CodeGen][CXX]: Fix no_destroy CG bug under specific circumstances
Sun, Nov 11, 5:21 PM
kristina closed D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..
Sun, Nov 11, 5:21 PM
kristina added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

Though on the other hand it makes no sense to skip it with asserts off either, that just clicked in my head, sorry.

Sun, Nov 11, 5:08 PM
kristina updated the diff for D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

Revised to address comments.

Sun, Nov 11, 5:04 PM
kristina added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

LGTM too, with one small fix in test. Thanks for working on this!

Sun, Nov 11, 5:02 PM
kristina closed D54380: [llvm-objdump] Add symbol 'O' for object data.

Grouped and linked all related commits. Closing.

Sun, Nov 11, 11:46 AM
kristina added 8 commit(s) for D54380: [llvm-objdump] Add symbol 'O' for object data: rLLD346612: [lld][test] Update tests using objdump, rL346611: [llvm][test] Update tests using objdump, rL346612: [lld][test] Update tests using objdump, rL346613: [ELF] Fix objdump tests after rL346610, rL346614: [ELF] Fix relocation-common.s after rL346610, rL346617: [MC] Fix 3 objdump tests after rL346610, rLLD346613: [ELF] Fix objdump tests after rL346610, rLLD346614: [ELF] Fix relocation-common.s after rL346610.
Sun, Nov 11, 11:42 AM
kristina added an edge to rLLD346612: [lld][test] Update tests using objdump: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added an edge to rLLD346613: [ELF] Fix objdump tests after rL346610: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added an edge to rLLD346614: [ELF] Fix relocation-common.s after rL346610: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added an edge to rL346614: [ELF] Fix relocation-common.s after rL346610: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added an edge to rL346611: [llvm][test] Update tests using objdump: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added an edge to rL346613: [ELF] Fix objdump tests after rL346610: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added an edge to rL346612: [lld][test] Update tests using objdump: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added an edge to rL346617: [MC] Fix 3 objdump tests after rL346610: D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 11:42 AM
kristina added a comment to D54380: [llvm-objdump] Add symbol 'O' for object data.

Last fixes in rL346614 and rL346617. Big sorry for the buildbot storm, I should have reverted it after the first set of failures and dissected it after, or better yet grepped for objdump usage by other targets when reviewing.

Sun, Nov 11, 11:37 AM
kristina added a comment to D54380: [llvm-objdump] Add symbol 'O' for object data.

About to fail again:

Sun, Nov 11, 11:03 AM
kristina added a comment to D54380: [llvm-objdump] Add symbol 'O' for object data.

rLLD346613 fixes MIPS LLD tests. Looking at build console, there is a bunch of other target specific tests that need updates after this.

Sun, Nov 11, 10:59 AM
kristina added a comment to D54380: [llvm-objdump] Add symbol 'O' for object data.

rL346611 for LLVM testsuite, and rLLD346612 for LLD testsuite should fix tests affected by this. Will close when/if llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast is happy.

Sun, Nov 11, 10:49 AM
kristina committed rLLD346612: [lld][test] Update tests using objdump.
[lld][test] Update tests using objdump
Sun, Nov 11, 10:47 AM
kristina committed rL346612: [lld][test] Update tests using objdump.
[lld][test] Update tests using objdump
Sun, Nov 11, 10:46 AM
kristina committed rL346611: [llvm][test] Update tests using objdump.
[llvm][test] Update tests using objdump
Sun, Nov 11, 10:44 AM
kristina reopened D54380: [llvm-objdump] Add symbol 'O' for object data.

This is causing some LLVM and LLD tests to fail on buildbots, since they rely on the old format.

Sun, Nov 11, 10:13 AM
kristina committed rL346610: [llvm-objdump] Add symbol 'O' for object data.
[llvm-objdump] Add symbol 'O' for object data
Sun, Nov 11, 9:50 AM
kristina closed D54380: [llvm-objdump] Add symbol 'O' for object data.
Sun, Nov 11, 9:50 AM
kristina added a comment to D54380: [llvm-objdump] Add symbol 'O' for object data.

LGTM with minor nitpick, wouldn't using CHECK-NEXT in the Mach-O test make more sense like in the other tests?

Thanks a lot, I will update it later.

Also, do you need it committed?

Yes, please

Sun, Nov 11, 8:52 AM
kristina added a comment to D54381: [llvm-exegesis] InstructionBenchmarkClustering::dbScan(): use llvm::SetVector<> instead of ILLEGAL std::unordered_set<>.

I could be missing something, but I don't understand why ToProcess needs to be a set-like container since we're erasing elements as we go (ie the erased elements won't be duplicate checked on next insertion). We skip any that have been previously processed in the inner loop too, which seems like it's doing the same work the set would be doing.

The isNoise() check also looks odd, since if CurrentCluster.Id has id kNoise then it could push the same index into CurrentCluster.PointIndices an unspecified number of times depending on the order that the elements are inserted and removed from ToProcess, but if CurrentCluster.Id can't be kNoise then that's not relevant.

From the docs:

SetVector is an adapter class that defaults to using std::vector

so calling erase on the first element isn't going to be terribly efficient either.

Sun, Nov 11, 8:17 AM
kristina accepted D54380: [llvm-objdump] Add symbol 'O' for object data.

LGTM with minor nitpick, wouldn't using CHECK-NEXT in the Mach-O test make more sense like in the other tests?

Sun, Nov 11, 6:26 AM

Sat, Nov 10

kristina added a comment to D54378: Add Hurd triplet to LLVMSupport.

I'm not sure to understand: do you mean to drop this patch entirely, so that llvm considers i386-pc-gnu as just a GNU-ish platform, and then Driver::getToolChain should special-case the Triple::UnknownOS and parse Triple::getTriple itself to call the Hurd code instead of the default toolchains::Generic_GCC case?

Sat, Nov 10, 5:48 PM
kristina added a comment to D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type.

The patch applies for me but has quite a few style violations. I'll fix those up before landing it.

Sat, Nov 10, 4:59 PM
kristina requested changes to D54378: Add Hurd triplet to LLVMSupport.

Perhaps we could go this way with additional code in cmake/config-ix.cmake, to get "hurd-gnu" as soon as possible just after detection and keep everything else inside llvm straight with llvm conventions?

Sat, Nov 10, 4:50 PM
kristina added a comment to D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type.

I can do it if you'd like, will be a moment though.

Thanks and much appreciated!

Sat, Nov 10, 4:25 PM
kristina added a dependent revision for D54378: Add Hurd triplet to LLVMSupport: D54379: Add Hurd toolchain support to Clang.
Sat, Nov 10, 3:47 PM
kristina added a dependency for D54379: Add Hurd toolchain support to Clang: D54378: Add Hurd triplet to LLVMSupport.
Sat, Nov 10, 3:47 PM
kristina updated the diff for D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

Revised to address nitpicks.

Sat, Nov 10, 3:15 PM
kristina added a comment to D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type.

Thanks!
I don't have commit access to land this myself.

Sat, Nov 10, 2:38 PM
kristina updated the diff for D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

@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 circumstances and two tests to ensure ctor is not balanced with either flag or attribute.

Sat, Nov 10, 2:24 PM
kristina added a comment to D54378: Add Hurd triplet to LLVMSupport.

If you absolutely must use this triple, however, I would suggest handling it in the Clang driver, you have slightly more leeway there, and there isn't going to be as much resistance since one of the main reasons we have the driver (aside from the obvious) is translating certain flags from what people are used to (or what's needed for compatibility) to an internal compiler command-line, aka -cc1 (or -cc1as). Again, support for GNU Hurd is very welcome here, it's just that we also have to make sure everyone else is happy, which includes downstream GNU Linux variations and various Glibc maintainers. I'm merely suggesting not registering the target with that triple in LLVMSupport which is kind of core to *all* parts of LLVM, while your interactions with Clang are mostly governed by the driver (you don't usually avoid the driver since it builds/adds/translates a lot of internal but required flags before invoking the frontend).

Sat, Nov 10, 9:03 AM
kristina updated subscribers of D54378: Add Hurd triplet to LLVMSupport.

You could use a different entry point for the driver similar to clang-cl or do what clang does on Darwin systems, that would avoid using this triple. What goes into clang driver is translated into an invocation of clang -cc1 frontend, which is where the -triple option is passed through. If you mess with the driver or just build your own invocation, and translate your triple to something that makes more sense internally (ie. i386-pc-mach-gnu) you have more leeway. Here however you're starting the process of adding a new experimental target to LLVM and in general my advice on new targets is to get some form of blessing from @chandlerc since I'd prefer for him to quickly glance over this. That's not to say I'm against adding support for GNU Hurd, I'm moreso implying that regardless of what GNU Hurd guidelines say, if they may cause problems in LLVM ecosystem, especially as far as core targets (or rather platforms) go, you have to consider the large ecosystems that use Clang and LLVM and sometimes adapt to avoid causing issues downstream (for example with distro vendors with Glibc/GNU Linux being the core of main variations of server and desktop Linux distros).

Sat, Nov 10, 8:40 AM
kristina added reviewers for D54379: Add Hurd toolchain support to Clang: kristina, Restricted Project.

A few style naming/comments.

Sat, Nov 10, 7:26 AM
kristina added a comment to D54378: Add Hurd triplet to LLVMSupport.

I'm not sure how GCC does it but this seems pretty out of line in comparison to all other targets, GNU makes sense to indicate that the target uses a GNU (usually with Glibc) userland but there's no indication of the kernel, which would be something like mk or mach. In which case I think i386-pc-mach-gnu or something along the lines would make sense, unless this is absolutely needed for compatibility reasons?

Hi Kristina,

Triples are not always triples, they're actually tuples, and that raises a huge number of issues, for example, in "target-pc-gnu", is that the environment, the OS or both?

Unfortunately, this mess exists for a very long time in toolchains that cannot be rebuilt in OSs that cannot choose a triple that is actually a quadruple.

From Hurd's own porting guidelines [1], "target-pc-gnu" is the triple they use, so there's not much we can do to change that.

The only problem from this patch is if people forget the OS part, for example, write "target-gnu" instead of "target-linux-gnu". Before, that'd throw an error, now it'll match with Hurd. It's hard to avoid hand typos in triples and have a sane code, but automated systems could end up generating "target--gnu" which is still parsed as Hurd.

Honestly, I don't think there is a way to fix this one other than asking Hurd to change their triple to "target-pc-hurd-gnu", but I don't think that would spawn a conversation that would end well. :)

cheers,
--renato

[1] https://www.gnu.org/software/hurd/hurd/porting/guidelines.html

Sat, Nov 10, 6:53 AM
kristina added inline comments to D54379: Add Hurd toolchain support to Clang.
Sat, Nov 10, 6:22 AM
kristina added a comment to D54378: Add Hurd triplet to LLVMSupport.

Hm, shouldn't it be in line with most other triples?

Sat, Nov 10, 6:13 AM
kristina updated the diff for D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

Revised, added a newline to the testcase.

Sat, Nov 10, 3:29 AM
kristina updated the diff for D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

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 not test this specific corner case, I've managed to manually narrow it down and write a reliable regression test. The core issue boils down to having a non-trivially destructible member in a superclass that lacks a destructor and a subclass that lacks a destructor, in which case, a different path was taken to balance out the static storage duration class' initializer call and the __cxa_atexit registration. This adds an explicit check for the attribute and avoids balancing out the constructor as intended by the attribute.

Sat, Nov 10, 3:14 AM
kristina added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

I've managed to isolate enough to make for a testcase. Is that too broad or is it okay to attach as is?

Sat, Nov 10, 2:03 AM
kristina committed rL346583: [clang]: Fix misapplied patch in 346582..
[clang]: Fix misapplied patch in 346582.
Sat, Nov 10, 12:07 AM
kristina committed rC346583: [clang]: Fix misapplied patch in 346582..
[clang]: Fix misapplied patch in 346582.
Sat, Nov 10, 12:07 AM

Fri, Nov 9

kristina committed rC346582: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC..
Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.
Fri, Nov 9, 11:56 PM
kristina committed rL346582: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC..
Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC.
Fri, Nov 9, 11:56 PM
kristina closed D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC..
Fri, Nov 9, 11:56 PM
kristina accepted D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC..
Fri, Nov 9, 11:55 PM
kristina created D54373: [clang]: Correct naming conventions and 80 col rule violation in CGDeclCXX.cpp. NFC..
Fri, Nov 9, 11:53 PM
kristina added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

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 somewhere, the repeated use of metaclass-like macros seem to break Clang's lexer/parser and leave it in a state where it starts behaving erratically, with this being a major symptom of it.

Fri, Nov 9, 11:09 PM
kristina added a comment to D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf..

I'm in favor of breaking changes for preserving GNU binutils compatibility. I think if downstream projects require "nonstandard" aliases, it is easy enough to tweak while in upstream I think compatibility is more important and brings LLVM's replacements for binutils one step closer to being drop-ins, which in turn encourages wider adoption of said tools.

Fri, Nov 9, 6:46 PM
kristina added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

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.

I'm going to echo this request -- without a test case, it's hard to know whether this actually fixes anything or just moves the issue elsewhere.

Fri, Nov 9, 3:28 PM
kristina added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

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.

Fri, Nov 9, 1:25 PM
kristina updated the diff for D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

Revised (style/ordering).

Fri, Nov 9, 12:18 PM
kristina created D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..
Fri, Nov 9, 12:14 PM

Thu, Nov 8

kristina added reviewers for D54203: Use template instead of void* -based type erasure in MachineRegistry: fedor.sergeev, chandlerc.

Pass manager related stuff, probably good idea to have Chandler or Fedor look over it since they've been doing a lot of work related to PM infrastructure.

Thu, Nov 8, 1:41 AM

Wed, Nov 7

kristina accepted D54127: Extend virtual file system with `isLocal` method..

LGTM.

Wed, Nov 7, 3:25 PM
kristina closed D53947: [WedAssembly] Add -s and -S alias for --strip-all and --strip-debug.

Closed by rL345767. Please mention the new revision or add "Differential Revision: rLXXXXXX" to the commit message so the commit is linked to the differential. Thanks.

Wed, Nov 7, 3:09 PM
kristina closed D53566: Fix warning: extra ‘;’ [-Wpedantic].

Closing it, was fixed in an unrelated revision.

Wed, Nov 7, 2:54 PM

Thu, Nov 1

kristina added inline comments to D53842: [WebAssembly] Parsing missing directives to produce valid .o.
Thu, Nov 1, 11:13 AM

Wed, Oct 31

kristina added a comment to D53804: [llvm-objdump] add support for '--reloc' as an alias of -r (PR39407).

Is this version ok? Shall I update other options flag in another patch? Thanks a lot for your advice

What you did is fine for now. I do think we should consider working on the command-line options a bit more in llvm-objdump. There are a few issues that I know of already, including https://bugs.llvm.org/show_bug.cgi?id=37895 (which covers the missing single-letter alias options in the help text) and https://bugs.llvm.org/show_bug.cgi?id=31679 (which points out that single-letter options don't combine properly (e.g. you can't do -fsr). The command-line options are also not GNU-compatible in some instances, in that they do different things. I'm going to raise a thread on the mailing list about this, if I get a chance, as to fix it, we'd have to change the meaning of some of the current options.

Wed, Oct 31, 3:11 AM
kristina committed rL345704: [llvm-objdump] Mark syms/t flags as NotHidden. NFC..
[llvm-objdump] Mark syms/t flags as NotHidden. NFC.
Wed, Oct 31, 2:38 AM
kristina committed rL345703: [llvm-objdump] Add --reloc alias for -r (PR39407).
[llvm-objdump] Add --reloc alias for -r (PR39407)
Wed, Oct 31, 2:36 AM
kristina closed D53804: [llvm-objdump] add support for '--reloc' as an alias of -r (PR39407).
Wed, Oct 31, 2:36 AM
kristina added a comment to D53804: [llvm-objdump] add support for '--reloc' as an alias of -r (PR39407).

Shall we commit this patch and then refactor alias options? I remember I've brought 3 alias options into llvm-objdump (include this one). They are: https://reviews.llvm.org/rL345495 https://reviews.llvm.org/rL345697

Wed, Oct 31, 1:41 AM
kristina added a comment to D53804: [llvm-objdump] add support for '--reloc' as an alias of -r (PR39407).

I think in this instance -r should not be hidden. And you'd need to override the default behavior by using cl::NotHidden. I spoke to Chandler and ideally we should start migrating common LLVM tools that substitute for binutils to a more user friendly llvm::Option based interface (kind of similar to what you mention regarding gnu-binutils) instead of the global cl:: registry which is really intended for global LLVM options, but that can be done later as part of larger refactoring effort (which I'll need to bring up on llvm-dev). But ideally for now, while we're still using cl:: I think it makes more sense if it's not hidden since it's a tool specific option.

Wed, Oct 31, 1:15 AM
kristina added a comment to D53804: [llvm-objdump] add support for '--reloc' as an alias of -r (PR39407).

Thank you everyone,

Hm, maybe it would be a good idea to make the shorthand versions visible (since cl::alias implies cl::Hidden by default)? I feel like these aliases shouldn't be buried all the way in --help-hidden since they're pretty common.

I agree with you. What if print help message like gnu-objdump? I think we could make it more like gnu-objdump

And here's another issue complain about --help option: https://bugs.llvm.org/show_bug.cgi?id=37895

Wed, Oct 31, 12:31 AM
kristina accepted D53804: [llvm-objdump] add support for '--reloc' as an alias of -r (PR39407).

Hm, maybe it would be a good idea to make the shorthand versions visible (since cl::alias implies cl::Hidden by default)? I feel like these aliases shouldn't be buried all the way in --help-hidden since they're pretty common.

Wed, Oct 31, 12:09 AM

Tue, Oct 30

kristina committed rL345697: [llvm-objdump] support '--syms' as an alias of -t.
[llvm-objdump] support '--syms' as an alias of -t
Tue, Oct 30, 10:47 PM