Page MenuHomePhabricator

jhenderson (James Henderson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 2:49 AM (98 w, 4 d)

Recent Activity

Fri, Dec 7

jhenderson accepted D55329: [llvm-readobj] Little clean up inside `parseDynamicTable`.

LGTM for this change. The refactor is certainly good. I feel like I might have suggested this in a previous review a while back, but it obviously never happened (see D49016).

Fri, Dec 7, 1:52 AM
jhenderson added inline comments to D55298: [llvm-readelf] Add -e/--headers support to readobj/elf.
Fri, Dec 7, 1:18 AM

Thu, Dec 6

jhenderson added a comment to D55298: [llvm-readelf] Add -e/--headers support to readobj/elf.

Full context? I use git format-patch -1 and upload the file it produces.

Thu, Dec 6, 6:21 AM
jhenderson added a comment to D55298: [llvm-readelf] Add -e/--headers support to readobj/elf.

The output from llvm-readobj vs. llvm-readelf is almost always different and that is either by design or accident. Are you suggesting syncing the output between readobj and readelf for this case, basically setting, --elf-output-style=GNU when --headers is specified. Is that what you want?

We shouldn't sync the output between llvm-readobj and llvm-readelf, although both should produce the right set of headers when requested. For llvm-readelf, we should match what GNU readelf does. llvm-readobj does not need to match this.

Thu, Dec 6, 2:30 AM

Wed, Dec 5

jhenderson requested changes to D55298: [llvm-readelf] Add -e/--headers support to readobj/elf.

Please remember to include the context when creating the diff with Phabricator (see the section starting "To get a full diff" here.

Wed, Dec 5, 2:37 AM
jhenderson accepted D55273: [test] Skip ThinLTO cache tests requiring atime setting on NetBSD.

LGTM. Thinking about it a bit more, and I don't think using the cache will cause any actual problems if everything has the same access time, beyond potentially doing unnecessary work.

Wed, Dec 5, 2:25 AM
jhenderson accepted D55271: [test] Split strip-preserve-time.test, and skip atime test on NetBSD.

LGTM.

Wed, Dec 5, 2:14 AM

Tue, Dec 4

jhenderson added a comment to D55273: [test] Skip ThinLTO cache tests requiring atime setting on NetBSD.

What would happen if the thin LTO cache is used on such a system? Do we need to put something in the thin LTO code to emit some sort of error, if this is attempted?

Tue, Dec 4, 8:37 AM
jhenderson accepted D55271: [test] Split strip-preserve-time.test, and skip atime test on NetBSD.

LGTM, with one minor comment.

Tue, Dec 4, 8:35 AM
jhenderson added a comment to D55271: [test] Split strip-preserve-time.test, and skip atime test on NetBSD.

Looks good, with a couple of nits.

Tue, Dec 4, 7:56 AM
jhenderson accepted D55220: [yaml2obj] Move redundant statements into a separate static function.

LGTM.

Tue, Dec 4, 6:28 AM
jhenderson added inline comments to D55220: [yaml2obj] Move redundant statements into a separate static function.
Tue, Dec 4, 4:46 AM
jhenderson added inline comments to D55220: [yaml2obj] Move redundant statements into a separate static function.
Tue, Dec 4, 1:45 AM
jhenderson requested changes to D55220: [yaml2obj] Move redundant statements into a separate static function.
Tue, Dec 4, 1:43 AM

Mon, Dec 3

jhenderson added inline comments to D54939: [RFC] [llvm-objcopy] Initial COFF support.
Mon, Dec 3, 5:31 AM
jhenderson accepted D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Mon, Dec 3, 3:54 AM
jhenderson added a comment to D54384: [llvm-objcopy] Add --build-id-link-dir flag.

LGTM, with a couple of minor comments.

Mon, Dec 3, 3:53 AM
jhenderson added a comment to D54939: [RFC] [llvm-objcopy] Initial COFF support.

I'm not really a COFF expert, so I haven't yet looked at the details of this change. It broadly looks like the right direction though.

Mon, Dec 3, 3:36 AM
jhenderson added a comment to D54867: [yaml2obj] Introduce symbol version section to yaml2obj.

I'm still not particularly familiar with yaml2obj, so my comments might not always make sense, so feel free to correct me.

Mon, Dec 3, 3:26 AM
jhenderson added a comment to D54939: [RFC] [llvm-objcopy] Initial COFF support.

i will get to this diff early next week, sorry about the delay. A small side note - probably the simple changes to include/llvm/Object/COFF.h can be factored out from this diff and reviewed separately (and probably much faster).

Ok - splitting them out isn't hard, but I dunno what to make as test for them, if they are going in as such.

Mon, Dec 3, 2:50 AM

Thu, Nov 29

jhenderson added a comment to D54867: [yaml2obj] Introduce symbol version section to yaml2obj.

Sorry, I've been a bit busy over the last couple of days. Where can I find documentation on these two sections you are adding support for?

Thu, Nov 29, 7:08 AM

Tue, Nov 27

jhenderson accepted D54936: [llvm-objcopy] Hook up the -V alias to --version, output "GNU strip".

I think it would make sense to test in both cases, honestly, but I'm not too fussed.

Tue, Nov 27, 4:40 AM
jhenderson added a comment to D54939: [RFC] [llvm-objcopy] Initial COFF support.

Please do split this into a separate pass-through change and option-hook up. It will keep the scale of the review down by quite a bit. Hooking up the options one or two at a time after that would then make reviewing each of those easier too.

Tue, Nov 27, 2:59 AM
jhenderson requested changes to D54936: [llvm-objcopy] Hook up the -V alias to --version, output "GNU strip".

Should we do the same thing (adding "comptabile with GNU objcopy") for llvm-objcopy as well as llvm-strip?

Tue, Nov 27, 2:53 AM
jhenderson added inline comments to D54384: [llvm-objcopy] Add --build-id-link-dir flag.
Tue, Nov 27, 2:49 AM
jhenderson added inline comments to D54877: Update coding guidelines regarding use of `auto`.
Tue, Nov 27, 2:27 AM

Mon, Nov 26

jhenderson added a comment to D54867: [yaml2obj] Introduce symbol version section to yaml2obj.

Whilst I'm not opposed to this in principle, I do wonder if the more correct thing to do is to make .gnu.version_r sections (which I assume have a distinctive ELF type) another kind of ELFYAML section (e.g. like RelocationSection).

Mon, Nov 26, 1:37 AM

Tue, Nov 20

jhenderson added a comment to D54697: [llvm-objdump] Add `Version References` dumper (WIP) (PR30241).

Could you add tests for the cases where a) there is no DT_VERNEEDNUM tag, and b) where it has a value of 0, please.

Tue, Nov 20, 3:52 AM
jhenderson added a comment to D54674: [llvm-objcopy] First bits for MachO .

I'd be interested to hear yours and other people's thoughts on not calling reportError inside the MachO layer and propagating it out at least to executeObjcopyOnBinary, via llvm::Error/Expected. I know @jakehehrlich was talking about this in the ELF side at one point too.

Tue, Nov 20, 3:22 AM

Mon, Nov 19

jhenderson added a comment to D54422: [ELF] - Do not ICF two sections with different output sections when using linker scripts.

I don't think this is necessarily a bug. At least, "predicating" the name of an output section does not seems a good idea to me. It is getting too tricky, and I don't like to add more complexity here. I generally do not encourage users use linker scripts as it makes linking slower and trickier, and it is to me an acceptable consequence that ICF folds input sections before linker scripts bin input sections to output sections.

Mon, Nov 19, 7:46 AM
jhenderson added a comment to D54651: [ELF] Allow --noinhibit-exec to produce corrupted executable with relocation overflow.

Functionality wise, this seems like a reasonable idea to me. Personally, I think I'd prefer --noinhibit-exec to more closely match the BFD approach of still emitting an actual error, but that's orthogonal to the issue this is trying to solve.

Mon, Nov 19, 1:34 AM
jhenderson added a comment to D54674: [llvm-objcopy] First bits for MachO .

I don't know anything about the MachO file format really, so I'm not sure I'm well qualified to review the file format side of this. I've made a couple of drive-by nitpick comments though, and may come back later with more substantial comments.

Mon, Nov 19, 1:19 AM

Thu, Nov 15

jhenderson accepted D54573: [llvm-objdump] Use `auto` declaration in typecasting.

LGTM.

Thu, Nov 15, 3:52 AM
jhenderson requested changes to D54384: [llvm-objcopy] Add --build-id-link-dir flag.

Sorry for not getting to this earlier. It's been a busy week for me.

Thu, Nov 15, 3:07 AM
jhenderson added inline comments to D54509: [llvm-objdump] Improve ELF file type checking statements.
Thu, Nov 15, 1:46 AM

Wed, Nov 14

jhenderson added a comment to D54520: [llvm-objdump] Using consistent arguments name.

I don't really mind either way. Don't make wholesale changes to coding styles to update it to LLVM's style though. If you want to change anything, I'd just change printELFFileHeader, but I don't think it's really worth it unless you are changing the area of code already.

Wed, Nov 14, 5:42 AM
jhenderson requested changes to D54520: [llvm-objdump] Using consistent arguments name.

If anything, the header should be updated to use "Obj" for one or two of the other cases. "o" does not fit LLVM's current coding standards, and also does not match the name used in the .cpp file (see ELFDump.cpp).

Wed, Nov 14, 4:45 AM
jhenderson added a comment to D54509: [llvm-objdump] Improve ELF file type checking statements.

Just as a tip, don't forget to put the Differential Revision field in your commit message. Then, when you commit it, the commit will automatically get associated with Phabricator, and you don't need to do anything manually, like close the revision or update the final diff. There are some instructions here: https://llvm.org/docs/Phabricator.html#committing-a-change. Also, take a look at commits made by various other people as well based on Phabricator reviews.

Wed, Nov 14, 4:31 AM
jhenderson added a comment to D54509: [llvm-objdump] Improve ELF file type checking statements.

LGTM. Do you need it committing? You've probably had enough commits by now that you could probably request commit access, if you want.

Wed, Nov 14, 3:22 AM
jhenderson added inline comments to D54509: [llvm-objdump] Improve ELF file type checking statements.
Wed, Nov 14, 3:05 AM

Nov 9 2018

jhenderson accepted D54299: [llvm-objdump] add more constraints for tests.

LGTM.

Nov 9 2018, 4:00 AM
jhenderson requested changes to D54293: [llvm-objdump] Simpify test command using.

I specifically asked for this test to be written this way.

Nov 9 2018, 2:05 AM
jhenderson accepted D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf..

Okay, from my point of view, LGTM (with a nit). However, we should probably get some more buy-in on the RFC before breaking people's builds etc, so you might want to hold off on the commit for now.

Nov 9 2018, 2:02 AM
jhenderson accepted D54238: [llvm-strings] Fix whitespaces to match strings output..

LGTM.

Nov 9 2018, 1:52 AM

Nov 8 2018

jhenderson added inline comments to D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf..
Nov 8 2018, 2:14 AM
jhenderson added a comment to D54238: [llvm-strings] Fix whitespaces to match strings output..

Could you add a test for --radix + --print-file-name, please, as the interaction could have changed with your implementation too. Otherwise looks good.

Nov 8 2018, 1:44 AM

Nov 7 2018

jhenderson added a comment to D54200: [PPC64] Use INT32_MIN instead of std::numeric_limits<int32_t>::min().

My preference is INT32_MIN which works both in C and C++, but I don't think 0x80000000 is more readable than numeric_limits<int32_t>::min(). Honestly it looks a bit more like an exercise to see if you really understand how negative numbers are represented in the two's complement, I think.

I have no objection to INT32_MIN, as it's clear as to the purpose.

Nov 7 2018, 5:53 AM
jhenderson added a comment to D54200: [PPC64] Use INT32_MIN instead of std::numeric_limits<int32_t>::min().

Is the problem here that we are missing a cast to int64_t?

Not sure if you're referring to the original problem, but I assume you are. The original issue was highlighting a bug in MSVC's C++11 compatibility, as the type of the (now replaced) integer literal should be a signed long long automatically (effectively an int64_t), but instead it became an unsigned int. Casting it to an int64_t explicitly would also have worked, I think.

Nov 7 2018, 4:04 AM
jhenderson added a comment to D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf..

Please also add the following aliases:

  • --syms is also a GNU readelf option as an alias for --symbols.
  • --dyn-syms is the GNU readelf version of --dyn-symbols.
  • --histogram is the GNU readelf long-form of -I/--elf-hash-histogram.
Nov 7 2018, 3:56 AM
jhenderson added inline comments to D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf..
Nov 7 2018, 2:43 AM
jhenderson accepted D53051: [llvm-tapi] initial commit, supports ELF text stubs.

Great, LGTM, but please get somebody else to sign off too.

Nov 7 2018, 2:31 AM
jhenderson added a comment to D54200: [PPC64] Use INT32_MIN instead of std::numeric_limits<int32_t>::min().

I'm honestly not really convinced that -0x80000000LL is more readable. I think it's a matter of personal taste. I think numeric_limits<int32_t>::min() is more explicit about what it's aiming to achieve. That being said, I really don't care that much, so if somebody else gives a LGTM, I don't mind.

Nov 7 2018, 1:59 AM

Nov 6 2018

jhenderson added a reviewer for D54124: [llvm-readelf] Make llvm-readelf more compatible with GNU readelf.: jhenderson.

Wow, that's some spooky timing. I didn't realise somebody else was thinking the exact same thing as me at the time! I brought this up on the mailing list earlier today, not realising that this review existed!

Nov 6 2018, 8:37 AM
jhenderson added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

I've got a few more nits, but otherwise I'm happy with this. Somebody else should definitely take a look too though.

Nov 6 2018, 2:45 AM

Nov 5 2018

jhenderson added a comment to D53945: [TextAPI] TBD Reader/Writer.

I haven't looked at all of this yet, by a long way, but here are a few comments on the first few files.

Nov 5 2018, 8:19 AM
jhenderson added inline comments to D53051: [llvm-tapi] initial commit, supports ELF text stubs.
Nov 5 2018, 3:26 AM

Nov 1 2018

jhenderson accepted D53983: [llvm-objcopy] For multiclass Eq, associate help text with --name= , not --name.

LGTM, although please make sure that the whitespace is tidied up a bit, to make the indentation consistent, and reduce the number of long lines.

Nov 1 2018, 10:11 AM
jhenderson accepted D53913: [llvm-objcopy] Support --{enable,disable}-deterministic-archives.

Might want to wait to see if @ruiu or anybody else has any other comments though.

Nov 1 2018, 10:05 AM
jhenderson accepted D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

LGTM.

Nov 1 2018, 8:58 AM
jhenderson accepted D53954: [llvm-strip] Support --keep and --strip-all-gnu from llvm-objcopy.

LGTM, with the typo fix.

Nov 1 2018, 3:35 AM
jhenderson accepted D53971: [llvm-objcopy] Use proper cases.

Nice spot. I obviously slipped a few times when reviewing! LGTM.

Nov 1 2018, 3:33 AM
jhenderson added a comment to D53051: [llvm-tapi] initial commit, supports ELF text stubs.

It would be good to see some doxygen comments in the header files.

Nov 1 2018, 3:29 AM
jhenderson added a comment to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

LGTM.

I haven't even been able to get the assembler to emit a STT_COMMON symbol. What does the assembly and command-line look like?

You need to pass in --elf-stt-common=yes, and AFAICT also need to be using GNU as (not sure if clang can produce STT_COMMON like this).

I tried, and I still ended up with an STT_OBJECT...! But it doesn't matter at this point.

Nov 1 2018, 2:32 AM
jhenderson added inline comments to D53913: [llvm-objcopy] Support --{enable,disable}-deterministic-archives.
Nov 1 2018, 2:26 AM

Oct 31 2018

jhenderson accepted D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

I haven't even been able to get the assembler to emit a STT_COMMON symbol. What does the assembly and command-line look like?

Oct 31 2018, 9:23 AM
jhenderson accepted D53691: Introduce bug life cycle documentation..

LGTM too.

Oct 31 2018, 6:33 AM
jhenderson added a comment to D53913: [llvm-objcopy] Support --{enable,disable}-deterministic-archives.

I do not see any problem of adding these flags for the sake of compatibility with GNU. What I wondered is whether or not you really have a need to make it nondeterministic. I thought that we might be able to just ignore these flags.

Oct 31 2018, 3:32 AM
jhenderson 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.

Oct 31 2018, 2:59 AM

Oct 30 2018

jhenderson committed rL345588: [llvm-size] Reject unknown radix values.
[llvm-size] Reject unknown radix values
Oct 30 2018, 4:55 AM
jhenderson closed D53799: [llvm-size] Reject unknown radix values.
Oct 30 2018, 4:55 AM
jhenderson committed rLLD345579: [ELF][PPC64]Workaround bogus Visual Studio build warning.
[ELF][PPC64]Workaround bogus Visual Studio build warning
Oct 30 2018, 3:58 AM
jhenderson committed rL345579: [ELF][PPC64]Workaround bogus Visual Studio build warning.
[ELF][PPC64]Workaround bogus Visual Studio build warning
Oct 30 2018, 3:58 AM
jhenderson closed D53821: [ELF][PPC64]Workaround bogus Visual Studio build warning.
Oct 30 2018, 3:58 AM
jhenderson added a comment to D53799: [llvm-size] Reject unknown radix values.

Great, thanks. Do you need me to commit it for you?

Oct 30 2018, 3:40 AM
jhenderson requested changes to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

Just doing this to ask for the test update I mentioned.

Oct 30 2018, 3:38 AM
jhenderson added a comment to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

On STT_COMMON, it looks like it only matters in fully linked files, since STT_COMMON symbols must always be in SHN_COMMON in relocatable objects. However, STT_COMMON can affect dynamic link-time semantics too, so I guess we may have to check for both. You should check GNU objcopy's behaviour in this case too (i.e. does an STT_COMMON symbol in a fully-linked ELF get affected or not by this option - I assume it is affected), and test accordingly. At the moment, the tests only test for the relocatable output case (i.e. where STT_COMMON is in SHN_COMMON), and we need cases for STT_COMMON not in SHN_COMMON.

Oct 30 2018, 3:28 AM
jhenderson added a comment to D53803: [llvm-objdump] add support for '--syms' as an alias of -t (PR39406).

I would suggest renaming the test to symbol-table-elf.test for consistency with other *-elf tests in this directory.

Oct 30 2018, 3:15 AM
jhenderson accepted D53733: [llvm-objcopy] Fix --keep-global-symbol/--globalize-symbol for undefined symbols..

LGTM.

Oct 30 2018, 2:57 AM

Oct 29 2018

jhenderson added a comment to D53821: [ELF][PPC64]Workaround bogus Visual Studio build warning.

Thanks for cleaning this up. LGTM.

We could also fix this by using INT32_MIN or std::numeric_limts<int32_t>::min(), if preferred

Personally I prefer either of those over the raw numeric literal.

Oct 29 2018, 10:06 AM
jhenderson updated the diff for D53821: [ELF][PPC64]Workaround bogus Visual Studio build warning.

Use numeric_limits instead of literal.

Oct 29 2018, 10:06 AM
jhenderson added a comment to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

Huh. I'd be interested to know WHY making a common symbol local is causing that effect, but I'm happy with this going in to fix it. You might want to prod @jakehehrlich though to be sure.

Oct 29 2018, 10:00 AM
jhenderson accepted D53799: [llvm-size] Reject unknown radix values.

LGTM with the suggested changes in the CHECK command names.

Oct 29 2018, 9:48 AM
jhenderson added a comment to D53821: [ELF][PPC64]Workaround bogus Visual Studio build warning.

http://lab.llvm.org:8011/builders/lld-x86_64-win7/builds/28829 is an example build showing the warning.

Oct 29 2018, 9:34 AM
jhenderson created D53821: [ELF][PPC64]Workaround bogus Visual Studio build warning.
Oct 29 2018, 9:32 AM
jhenderson committed rL345503: [llvm-objdump] Don't crash when using `-a` on non-archives.
[llvm-objdump] Don't crash when using `-a` on non-archives
Oct 29 2018, 7:19 AM
jhenderson closed D53690: [llvm-objdump] Don't Crash When Using `-a` on Non-Archives (PR39402).
Oct 29 2018, 7:19 AM
jhenderson added a comment to D53690: [llvm-objdump] Don't Crash When Using `-a` on Non-Archives (PR39402).

Do you need help committing this again? If so, could you confirm how I should write your name in the attribution, please? (e.g. Xing Guo/Guo Xing etc)

Oct 29 2018, 5:26 AM
jhenderson added inline comments to D53691: Introduce bug life cycle documentation..
Oct 29 2018, 5:01 AM
jhenderson accepted D53690: [llvm-objdump] Don't Crash When Using `-a` on Non-Archives (PR39402).

LGTM, with the nit.

Oct 29 2018, 4:41 AM
jhenderson added inline comments to D53690: [llvm-objdump] Don't Crash When Using `-a` on Non-Archives (PR39402).
Oct 29 2018, 4:11 AM
jhenderson added inline comments to D53803: [llvm-objdump] add support for '--syms' as an alias of -t (PR39406).
Oct 29 2018, 4:01 AM
jhenderson requested changes to D53799: [llvm-size] Reject unknown radix values.

Code change is fine, but please add a test to show that an unknown value is rejected with a useful error message.

Oct 29 2018, 3:55 AM
jhenderson added a comment to D53782: [llvm-objcopy] Don't apply --localize flags to common symbols.

I can certainly understand your reasoning, but this could also adversely impact a desired use-case of hiding a symbol in an object or library from another object, which is presumably the purpose of --localize-hidden, and is what is discussed in https://chromium.googlesource.com/external/dynamorio/+/master/core/CMakeLists.txt#541:

Oct 29 2018, 3:47 AM
jhenderson added a comment to D53733: [llvm-objcopy] Fix --keep-global-symbol/--globalize-symbol for undefined symbols..

My feeling is that a single separate test is not the right way to test this. I think it makes more sense for an undefined symbol to be added to globalize.test and --keep-global-symbols.test. What do you think?

Oct 29 2018, 3:22 AM
jhenderson committed rL345495: [llvm-objdump] Add '--full-contents' as alias for '-s'.
[llvm-objdump] Add '--full-contents' as alias for '-s'
Oct 29 2018, 3:08 AM
jhenderson closed D53576: [llvm-objdump] Add alias option `--full-contents` for `-s` (PR39404) .
Oct 29 2018, 3:07 AM
jhenderson added a comment to D53576: [llvm-objdump] Add alias option `--full-contents` for `-s` (PR39404) .

Sorry for the delay, I was off sick. I will be committing this shortly.

Oct 29 2018, 2:33 AM

Oct 24 2018

jhenderson added a comment to D53576: [llvm-objdump] Add alias option `--full-contents` for `-s` (PR39404) .

Updated with clang-formatted codes, Please help me commit it, thanks a lot :)

Oct 24 2018, 9:17 AM
jhenderson accepted D53576: [llvm-objdump] Add alias option `--full-contents` for `-s` (PR39404) .

Do you need me to commit it?

Oct 24 2018, 8:48 AM
jhenderson added a comment to D53576: [llvm-objdump] Add alias option `--full-contents` for `-s` (PR39404) .

Hi, I upload a new test. Did I miss something? Thanks a lot

Oct 24 2018, 7:52 AM