This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Make lld::getInteger() tolerate leading "0x"/"0X" when base is 16
ClosedPublic

Authored by gkm on Sep 21 2020, 7:06 PM.

Details

Summary

ld64 is cool with leading 0x for hex command-line args, and we should be also.

Diff Detail

Event Timeline

gkm created this revision.Sep 21 2020, 7:06 PM
Herald added a project: Restricted Project. · View Herald Transcript
gkm requested review of this revision.Sep 21 2020, 7:06 PM
int3 added a subscriber: int3.Sep 21 2020, 7:17 PM

presumably this is related to D88064... could you reference it in the commit message and test the 0X case in D88064? (Actually, I've actually never seen 0X in the wild... does ld64 support it too?)

int3 added a comment.Sep 21 2020, 7:19 PM

nvm, I see all the hex values in D88064 aren't actually strings that we parse... but I'm still curious to know how this is going to be used / tested

gkm edited the summary of this revision. (Show Details)Sep 21 2020, 7:22 PM
gkm edited the summary of this revision. (Show Details)
gkm added a comment.EditedSep 21 2020, 7:29 PM

I just added the rationale to the diff summary.

While inspecting ld64, I leaned that it uses strtoull(3) for parsing hex command-line args, and strtoull(3) does this: " If base is zero or 16, the string may then include a "0x" prefix, and the number will be read in base 16"

How will it be used? Somewhere in the wild, we will avoid bugs.

gkm updated this revision to Diff 293321.Sep 21 2020, 7:34 PM
  • add a test case
int3 accepted this revision.Sep 21 2020, 7:37 PM

Thanks for the explanation!

lld/test/MachO/headerpad.s
24

wouldn't hurt to test the 0X case too

This revision is now accepted and ready to land.Sep 21 2020, 7:37 PM
MaskRay added inline comments.Sep 21 2020, 8:48 PM
lld/Common/Args.cpp
39

If base == 0, to_integer can parse the 0x prefix.

gkm updated this revision to Diff 293478.Sep 22 2020, 8:56 AM
gkm marked an inline comment as done.
  • add testcase for 0X prefix alongside 0x
  • resequence diff stack so this can land now
This revision was landed with ongoing or failed builds.Sep 22 2020, 8:56 AM
This revision was automatically updated to reflect the committed changes.

??????

If base == 0, to_integer can parse the 0x prefix.

This is unaddressed. Please see StringRef.cpp GetAutoSenseRadix.

gkm added a comment.Sep 22 2020, 1:37 PM

I only skip 0x when base == 16. My change does nothing to alter the base == 0 case, and GetAutoSenseRadix continues to operate as before.

gkm marked an inline comment as done.Sep 22 2020, 2:12 PM
gkm added inline comments.Sep 22 2020, 2:34 PM
lld/Common/Args.cpp
39

I am unclear about the intention of your statement. Are you suggesting that I change base to 0 and let to_integer() strip the 0x ?
I considered that, but that way we perform the 0x detection twice.

MaskRay added inline comments.Sep 22 2020, 5:01 PM
lld/Common/Args.cpp
39

If you use base 0, to_integer does not need a change and you get 0x ability for free.

gkm added inline comments.Sep 22 2020, 5:34 PM
lld/Common/Args.cpp
37–39

Is this what you prefer?