[PATCH] D65680: [yaml2obj][tests] Fix overly restrictive od output check

Mon Aug 5 00:29:31 PDT 2019

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

This looks good to me. Please wait to see if Fangrui is happy too.

================
Comment at: test/tools/yaml2obj/elf-header-sh-fields.yaml:50
+# NEWSIZE: {{^[^[:blank:]]+}} 01
+# OLDSIZE: {{^[^[:blank:]]+}} 40

----------------
hubert.reinterpretcast wrote:
> MaskRay wrote:
> > hubert.reinterpretcast wrote:
> > > grimar wrote:
> > > > Previously it tested that `e_shentsize` in ELF header at offset 0x72 contained value 64 (0x40) and with this YAML we changed it to 0x01.
> > > > But your version just test that somewhere in the file there are bytes 0x01 and 0x40 I think. Am I missing something?
> > > > But your version just test that somewhere in the file there are bytes 0x01 and 0x40 I think.
> > > My version tests that there is a byte whose value is 0x01 or 0x40 (for `NEWSIZE` and `OLDSIZE`, respectively) at offset 0x3a (octal 072).
> > I think it may be fine to just delete `{{^[^[:blank:]]+}}`. `-j 0x3a -N 1` implies there is only one byte.
> > 
> > @hubert.reinterpretcast What does `od -t x1 -v -j 0x3a -N 1 %t2 -A x` (note `-A x`) do on AIX?
> > 
> > If the address is still different, I would prefer `# OLDSIZE: 40` (I'm happy to change if @grimar has a different opinion)
> Removing the `{{^[^[:blank:]]+}}` risks the offset matching the byte content we are checking for. It's okay to do alongside `-A n`, and I can update the patch to do that if it is preferred.
> 
> The address with `-A x` is still different on AIX:
> ```
> 0000000  40
> 0000001
> ```
> 
> Note that placing the `-A x` after the file operand does not change the input offset base on AIX (similar to how `POSIXLY_CORRECT` behaves for some implementations).
>>But your version just test that somewhere in the file there are bytes 0x01 and 0x40 I think.
>
>My version tests that there is a byte whose value is 0x01 or 0x40 (for NEWSIZE and OLDSIZE, respectively) at offset 0x3a (octal 072).

Yes, sorry, I missed the `-j` option used here and in the committed patch.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65680