[PATCH] D79013: [llvm-readelf] - Do not crash when the PT_INTERP has an invalid offset.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with the two error message fixes. I'd appreciate @thopre's input on the numeric variable usage too, but that shouldn't block this landing.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test:322
+# ERROR-INTERP:      Type           Offset
+# ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET:0x000230]]
+# ERROR-INTERP-NEXT:     [Requesting program interpreter: C]
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > I believe you don't want the leading 0x000 bit, and then you also won't need the %x either. I believe FileCheck will automatically recognise it as hex that way (as things stand, I think it might not, but can't remember the details). If you want ot be sure that it is hex though, you can keep the %x (and might need it in the lines below too to be sure its always hex, but I'm not sure - experimentation needed). FWIW, I don't think it being hex is a necessary check for this specific case, since we check it earlier.
> > > > I believe you don't want the leading 0x000 bit, and then you also won't need the %x either.
> > > 
> > > It does not work for me, this was the only way I've managed to make this work.
> > > 
> > > The following fail:
> > > 
> > > 
> > > ```
> > > # ERROR-INTERP:      Type           Offset
> > > # ERROR-INTERP-NEXT: INTERP         [[#OFFSET:]]
> > > # ERROR-INTERP-NEXT:     [Requesting program interpreter: C]
> > > # ERROR-INTERP-NEXT: INTERP         [[#OFFSET + 1]]
> > > # ERROR-INTERP-NEXT:     [Requesting program interpreter: ]
> > > ```
> > > 
> > > ```
> > > D:\Work3\LLVM\llvm-project\llvm\test\tools\llvm-readobj\ELF\gnu-phdrs.test:324:2
> > > 2: error: ERROR-INTERP-NEXT: expected string not found in input
> > > # ERROR-INTERP-NEXT: INTERP [[#OFFSET + 1]]
> > >                      ^
> > > <stdin>:10:2: note: scanning from here
> > >  INTERP 0x000231 0x0000000000000000 0x0000000000000000 0x000000 0x000000 0x1
> > >  ^
> > > <stdin>:10:2: note: with "OFFSET + 1" equal to "1"
> > >  INTERP 0x000231 0x0000000000000000 0x0000000000000000 0x000000 0x000000 0x1
> > >  ^
> > > ```
> > Ah, okay, I guess the 0x is not cosnidered part of the numeric pattern and the default only reads decimal. @thopre, is that correct?
> > 
> > Anyway, from my experiments the leading zeroes can be included, and please specify '%x' explicitly in the definition.
> Just in case to confirm that my understanding about "leading zeroes" was correct here,
> here is a result of my experiments:
> 
> The following works:
> 
> ```
> # ERROR-INTERP:      Type           Offset
> # ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET:0x230]]
> ```
> 
> ```
> 
> # ERROR-INTERP:      Type           Offset
> # ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET:0x000230]]
> ```
> 
> The following doesn't:
> 
> ```
> # ERROR-INTERP:      Type           Offset
> # ERROR-INTERP-NEXT: INTERP         0x[[#%x,OFFSET:0x230]]
> ```
> 
> ```
> # ERROR-INTERP:      Type           Offset
> # ERROR-INTERP-NEXT: INTERP         0x[[#%x,OFFSET:230]]
> ```
> 
> So we need the "0x000[[" part, but do not need leading zeroes for `OFFSET`.
I missed the bit where you were explicitly specifying the value to capture and didn't replicate it in my own experiments properly. I'm at a loss here. The problem is that if the layout changes to have fewer (or more) leading zeroes, it will fail due to that.

That seems to be missing the point of numeric variables. Leading zeroes should be able to be included in the match.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/gnu-phdrs.test:329
+# ERROR-INTERP-NEXT: INTERP         0x000[[#%x,OFFSET + 3]]
+# ERROR-INTERP-NEXT: warning: '[[FILE]]': unable to read program interpreter name at offset 0x[[#%x,OFFSET+3]]: it goes past the end of the file (0x233)
+# ERROR-INTERP-NEXT: INTERP         0xaabbccddeeff1122
----------------
Can you not get rid of the %x in all the usages? I thought that should be derived from the definition of `OFFSET` if unspecified and only needed specifying if different from the original.

Also, there are still two instances of explicit "0x233" in these messages.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79013/new/

https://reviews.llvm.org/D79013