rupprecht (Jordan Rupprecht)
Engineering

Projects

User does not belong to any projects.

User Details

User Since
Jun 28 2018, 11:39 AM (6 w, 5 d)

Recent Activity

Today

rupprecht created D50744: [llvm-strip] Add support for -p/--preserve-dates.
Tue, Aug 14, 4:18 PM
rupprecht added a comment to D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..

What happens if you specify a mixture of --globalize-symbol and --keep-global-symbol?

The symbol will be a global in the output file.
That's how it accidentally was in the way I wrote it, but I added a test case to make sure it stays that way and left some more comments.

I think I wasn't explicit enough with my wording to explain what I was thinking: what happens if you specify --globalize-symbol a --keep-global-symbol b? It looks like the code will turn a into STB_LOCAL, and b will remain as it currently is (similar points about --localize|weaken-symbol also apply). Does this match GNU's behaviour? I'd expect a to be STB_GLOBAL and b to remain unchanged.

Tue, Aug 14, 3:04 PM
rupprecht updated the diff for D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..
  • Handle --globalize --keep-global correctly, and clean up comments
Tue, Aug 14, 3:04 PM
rupprecht added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Tue, Aug 14, 11:51 AM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Fix tests, add null symbol to symtab, make binary objection creation less fragile, and automatically assign section indices
Tue, Aug 14, 11:50 AM

Yesterday

rupprecht committed rL339628: [Support] NFC: Allow modifying access/modification times independently in sys….
[Support] NFC: Allow modifying access/modification times independently in sys…
Mon, Aug 13, 4:04 PM
rupprecht closed D50521: [Support] NFC: Allow modifying access/modification times independently in sys::fs::setLastModificationAndAccessTime..
Mon, Aug 13, 4:04 PM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Rebase/fix other method names to be lower cased.
Mon, Aug 13, 3:05 PM
rupprecht added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Mon, Aug 13, 3:05 PM
rupprecht committed rL339616: [llvm-objcopy] NFC: Fix minor formatting issues.
[llvm-objcopy] NFC: Fix minor formatting issues
Mon, Aug 13, 2:31 PM
rupprecht updated the diff for D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..
  • Misc updates to tests/comments
Mon, Aug 13, 2:18 PM
rupprecht added a comment to D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..

What happens if you specify a mixture of --globalize-symbol and --keep-global-symbol?

Mon, Aug 13, 2:18 PM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • echo -n -> echo where it doesn't matter
Mon, Aug 13, 11:28 AM

Fri, Aug 10

rupprecht added a comment to D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..

Sorry for the slew of reviews. I have another one for implementing "llvm-strip -p" almost ready to go too (about the same size as this), hopefully I'll be able to send it next week.

Fri, Aug 10, 5:06 PM
rupprecht created D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s)..
Fri, Aug 10, 4:47 PM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..

Rebase

Fri, Aug 10, 11:38 AM
rupprecht added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Fri, Aug 10, 11:36 AM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Fold arch-specific file header tests into a single templated test
  • Replace printf w/ echo -n
  • Other code review comments
Fri, Aug 10, 11:35 AM
rupprecht committed rL339448: [llvm-objcopy] NFC: consistently use typename ELFT::<X> definitions in headers.
[llvm-objcopy] NFC: consistently use typename ELFT::<X> definitions in headers
Fri, Aug 10, 9:26 AM

Thu, Aug 9

rupprecht committed rL339404: [llvm-objcopy] NFC: Add some color to error().
[llvm-objcopy] NFC: Add some color to error()
Thu, Aug 9, 3:52 PM
rupprecht added inline comments to D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..
Thu, Aug 9, 3:16 PM
rupprecht updated the diff for D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..
  • Un-revert divtc3 (128-bit fp), revert divxc3 (80-bit fp)
Thu, Aug 9, 3:16 PM
rupprecht added a comment to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..

Thanks for the review!

Thu, Aug 9, 11:05 AM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Add Elf_Sym TODO
Thu, Aug 9, 11:03 AM
rupprecht added a reviewer for D50521: [Support] NFC: Allow modifying access/modification times independently in sys::fs::setLastModificationAndAccessTime.: saugustine.
Thu, Aug 9, 10:44 AM
rupprecht created D50521: [Support] NFC: Allow modifying access/modification times independently in sys::fs::setLastModificationAndAccessTime..
Thu, Aug 9, 10:43 AM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Consistently use ELFT typename aliases in headers
Thu, Aug 9, 9:30 AM
rupprecht added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Thu, Aug 9, 9:22 AM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Extract out ehdr initialization into initEhdr()
Thu, Aug 9, 9:21 AM

Wed, Aug 8

rupprecht added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Wed, Aug 8, 3:35 PM
rupprecht added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Wed, Aug 8, 3:10 PM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Add sparc/ppc to arch map
Wed, Aug 8, 3:01 PM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Remove Ident from object
Wed, Aug 8, 2:39 PM
rupprecht added inline comments to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Wed, Aug 8, 2:39 PM
rupprecht added a comment to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.

Yeah, it's not my first choice either, but it did make the implementation easier and seemed somewhat consistent with the pattern elsewhere in this file.

Wed, Aug 8, 1:14 PM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Pass MachineInfo into BinaryReader to push ELFType template down to only BinaryELFBuilder<>
  • Move getElfType out of Reader (and also ELFReader), it can be implemented purely in the driver class. This make it clear that "Reader" is not tied to ELF types, even if that's all that's supported currently.
Wed, Aug 8, 1:06 PM
rupprecht abandoned D49330: [compiler-rt] Include -lm when using compiler-rt, due to dependencies in some __div methods..
Wed, Aug 8, 9:17 AM
rupprecht abandoned D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..

D50343 includes (some of) the changes here, so I'm officially abandoning this -- it makes more sense to discuss the design on something concrete.

Wed, Aug 8, 9:16 AM

Mon, Aug 6

rupprecht added a comment to D50343: [llvm-objcopy] Add support for -I binary -B <arch>..

Ah so I think I'm against BinaryReader being templated against ELFT. That is required to implement getElfType() however. getElfType drives which writer we create. That shouldn't be how things work however. I'll go and add my related comments to this to the previous patch. Sorry if I didn't see what was going on in the other patch and lead you astray. This patch makes things a bit more clear.

Mon, Aug 6, 3:28 PM
rupprecht updated the diff for D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
  • Refactor Object based on code review comments
Mon, Aug 6, 3:18 PM
rupprecht created D50343: [llvm-objcopy] Add support for -I binary -B <arch>..
Mon, Aug 6, 11:34 AM

Fri, Aug 3

rupprecht planned changes to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..

I think the changes here will make more sense with the implementation of BinaryReader::create(), so I'll hold off on this change for now. I'll revive it if it still makes sense to submit independently, but more likely I'll just roll this up into the next change. Sorry for the noise!

Fri, Aug 3, 10:55 AM

Thu, Aug 2

rupprecht updated the diff for D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
  • Revert [rename binary -> blob]
Thu, Aug 2, 1:02 PM
rupprecht added inline comments to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Thu, Aug 2, 1:01 PM
rupprecht committed rL338750: [LLD] Update split stack support to handle more generic prologues. Improve….
[LLD] Update split stack support to handle more generic prologues. Improve…
Thu, Aug 2, 11:14 AM
rupprecht committed rLLD338750: [LLD] Update split stack support to handle more generic prologues. Improve….
[LLD] Update split stack support to handle more generic prologues. Improve…
Thu, Aug 2, 11:14 AM
rupprecht closed D49926: Update split stack support to handle more generic prologues. Improve error handling. Add test file for better code-coverage. Update tests to be more complete..
Thu, Aug 2, 11:14 AM

Wed, Aug 1

rupprecht added inline comments to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Wed, Aug 1, 3:26 PM
rupprecht added inline comments to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Wed, Aug 1, 2:30 PM
rupprecht updated the diff for D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
  • Rename binary -> blob
Wed, Aug 1, 2:24 PM
rupprecht committed rL338635: [llvm-objcopy] Add missing -I command line flag alias for --input-target.
[llvm-objcopy] Add missing -I command line flag alias for --input-target
Wed, Aug 1, 2:00 PM
rupprecht updated the summary of D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Wed, Aug 1, 12:00 PM
rupprecht updated the diff for D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
  • Add llvm_unreachable to handle missing optional
Wed, Aug 1, 11:59 AM
rupprecht updated the diff for D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
  • Move Reader dependency out of CreateWriter
Wed, Aug 1, 11:55 AM
rupprecht added inline comments to D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Wed, Aug 1, 11:54 AM
rupprecht committed rL338582: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
[llvm-objcopy] Add support for --rename-section flags from gnu objcopy
Wed, Aug 1, 9:23 AM
rupprecht closed D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
Wed, Aug 1, 9:23 AM
rupprecht updated the diff for D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

Tweak flag message string

Wed, Aug 1, 9:17 AM
rupprecht added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

Okay, maybe here's my best, hopefully uncontroversial, suggestion, since it would be good to get this in for the LLVM 7.0 cut today(?): leave them in but mark them as "legacy" or for GNU compatibility e.g: Flags supported for GNU compatibility: alloc, load... in both error message and help text. If and when we add more flags, for a better/clearer UI, we can make that a separate sentence like Supported Flags: alloc, write, execinstr,... Additional flags supported for GNU compatibility: load, noload... etc.

That wording sounds good to me. I've updated the errors/tests.

LGTM with those changes.

Wed, Aug 1, 9:16 AM

Tue, Jul 31

rupprecht created D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader..
Tue, Jul 31, 4:37 PM
rupprecht updated the diff for D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
  • Cover full OS/proc flags in test, fix other comments
Tue, Jul 31, 11:12 AM
rupprecht added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

The code looks correct to me (barring a couple of nits) for what you are trying to do, but I do have a couple of outstanding concerns, that I'd like to discuss further:

I'm really worried about clearing the TLS flag, and to a lesser extent, the INFO_LINK flag. Clearing the TLS flag would almost certainly break the file, and I have no idea what the repercussions of clearing INFO_LINK might be. I'd be tempted to break with GNU compatibility here and not clear either of them. If we really don't want to do that, I'd recommend including a "tls" input option. I'm not sure what to do with INFO_LINK, because it affects other fields in the section header. Are you sure that the INFO_LINK and TLS flags are being cleared by the renaming and not by something else (e.g. implicit meaning of section names)? What happens if you rename e.g. .tdata.foo to .tdata.bar (thus maintaining the standard TLS naming scheme)?

I'm still not able to get SHF_TLS propagated, even with a real object file (not just something artificially created w/ yaml2obj):

Tue, Jul 31, 11:12 AM

Mon, Jul 30

rupprecht updated the diff for D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
  • Preserve certain SHF_* flags and tidy up error messages
Mon, Jul 30, 2:40 PM
rupprecht added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

My main concern is still the interaction between these switches and existing flags. Specifically, I'm worried about what will happen if this is used on a section with lots of flags that cannot be controlled via this switch. A quick look at the spec shows that this will clear the following flags unconditionally: SHF_INFO_LINK, SHF_GROUP, SHF_TLS, SHF_COMPRESSED, and any OS/processor-specific flags. I haven't checked what GNU objcopy does, but in basically all of those, changing the flags could be bad, and have a negative impact on the ability to link the resultant object. Could you investigate what GNU does in each of these cases, and write tests to show the behaviour, please.

Mon, Jul 30, 2:40 PM

Fri, Jul 27

rupprecht updated the diff for D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
  • Change error messages to match gnu binutils
Fri, Jul 27, 4:33 PM
rupprecht added inline comments to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
Fri, Jul 27, 4:32 PM
rupprecht committed rCXX338157: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4.
[libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4
Fri, Jul 27, 1:07 PM
rupprecht committed rL338157: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4.
[libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4
Fri, Jul 27, 1:05 PM
rupprecht closed D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4.
Fri, Jul 27, 1:05 PM
rupprecht added a comment to D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4.

Just to make sure I understand properly: this means we will use newlib's implementation of iswcntrl_l & friends instead of our own emulation (which is an ODR violation currently going unnoticed)? And this is OK because newlib provides iswcntrl_l & friends starting at version 2.4.

Yes, that's correct.
I clarified the description: when the methods were added, the tree was configured with NEWLIB_MINOR_VERSION = 4, meaning this wasn't released until the next version, newlib 2.5.

Fri, Jul 27, 12:24 PM
rupprecht updated the summary of D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4.
Fri, Jul 27, 12:18 PM
rupprecht updated the summary of D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4.
Fri, Jul 27, 12:07 PM
rupprecht created D49927: [libc++] Exclude posix_l/strtonum fallback inclusion for newlib > 2.4.
Fri, Jul 27, 12:06 PM
rupprecht updated the diff for D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
  • Clean up long test case, fix some naming
Fri, Jul 27, 10:15 AM
rupprecht added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

What is the behaviour of --rename-section when it touches a section with existing flags that don't conflict with the specified value? For example alloc when the section has SHF_EXECINSTR already? Does it preserve those flags?

It looks like SHF_EXECINSTR is not preserved in that case, e.g.

Fri, Jul 27, 10:15 AM

Thu, Jul 26

rupprecht updated the diff for D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

Refactored/addressed code review comments

Thu, Jul 26, 1:58 PM
rupprecht added a comment to D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.

Thanks for the review! I'm clearly still ramping up on this codebase :)

Thu, Jul 26, 1:57 PM
rupprecht created D49870: [llvm-objcopy] Add support for --rename-section flags from gnu objcopy.
Thu, Jul 26, 11:54 AM

Wed, Jul 25

rupprecht updated the diff for D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..
  • Exclude 80-bit floating point and fix ppc comment
Wed, Jul 25, 1:11 PM
rupprecht added inline comments to D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..
Wed, Jul 25, 1:11 PM
rupprecht updated the summary of D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..
Wed, Jul 25, 12:41 PM
rupprecht updated the diff for D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..
  • Inline methods to fp_lib.h and fix the ppc logb call
Wed, Jul 25, 12:36 PM
rupprecht added a comment to D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..

Maybe just make this a static inline function in fp_lib.h?

Thanks, done. That also makes these symbols not exported by the library, which is nice.

Wed, Jul 25, 12:35 PM

Tue, Jul 24

rupprecht committed rC337850: Revert "[VFS] Cleanups to VFS interfaces.".
Revert "[VFS] Cleanups to VFS interfaces."
Tue, Jul 24, 1:28 PM
rupprecht committed rL337850: Revert "[VFS] Cleanups to VFS interfaces.".
Revert "[VFS] Cleanups to VFS interfaces."
Tue, Jul 24, 1:28 PM
rupprecht added a comment to D49724: [VFS] Cleanups to VFS interfaces..

Looks like this caused some test failures (http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/14441/steps/ninja%20check%201), e.g.:

Tue, Jul 24, 1:22 PM

Mon, Jul 23

rupprecht added a comment to D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..

Ping -- Eli, do you have time to review this, or add someone else that can take a look if you're busy?

Mon, Jul 23, 12:12 PM
rupprecht committed rL337713: OpChain has subclasses, so add a virtual destructor..
OpChain has subclasses, so add a virtual destructor.
Mon, Jul 23, 10:38 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Mon, Jul 23, 10:38 AM
rupprecht created D49681: OpChain has subclasses, so add a virtual destructor..
Mon, Jul 23, 10:33 AM

Fri, Jul 20

rupprecht committed rL337604: [llvm-objcopy] Add basic support for --rename-section.
[llvm-objcopy] Add basic support for --rename-section
Fri, Jul 20, 12:59 PM
rupprecht closed D49576: [llvm-objcopy] Add basic support for --rename-section.
Fri, Jul 20, 12:59 PM
rupprecht added a comment to D49576: [llvm-objcopy] Add basic support for --rename-section.

Thanks for the suggestion; removing the Object method will probably make the next change (updating flags) simpler.

Fri, Jul 20, 11:23 AM
rupprecht updated the diff for D49576: [llvm-objcopy] Add basic support for --rename-section.

Revert changes to Object; do the renaming in objcopy directly.

Fri, Jul 20, 11:20 AM
rupprecht updated the diff for D49576: [llvm-objcopy] Add basic support for --rename-section.

Fixed indentation

Fri, Jul 20, 10:49 AM

Thu, Jul 19

rupprecht added inline comments to D49576: [llvm-objcopy] Add basic support for --rename-section.
Thu, Jul 19, 5:10 PM
rupprecht updated the diff for D49576: [llvm-objcopy] Add basic support for --rename-section.

Addressed all comments

Thu, Jul 19, 5:07 PM
rupprecht updated the diff for D49576: [llvm-objcopy] Add basic support for --rename-section.

Fixing no trailing \n in a test

Thu, Jul 19, 4:50 PM
rupprecht created D49576: [llvm-objcopy] Add basic support for --rename-section.
Thu, Jul 19, 4:48 PM

Wed, Jul 18

rupprecht updated subscribers of D49514: [compiler-rt] [builtins] Add logb/logbf/logbl methods to compiler-rt to avoid libm dependencies when possible..
Wed, Jul 18, 3:06 PM