IssueTracker

Err, I have no idea how gerrit changesets are reflected in the bug report, but there is a preliminary patch https://android-review.googlesource.com/#/c/159020/

Scope of changes to bionic are somewhat more complex than initially thought.

The troublesome spots are mmap() and friends, malloc() and friends (due to symbol clash between struct, fieldname, and function names) as well as most of the routines in string.h

string.h exposes some issues, because the routines in it are both defined as inline functions that call out to assembly counterpart of the same name using an inconsistent __builtin_XXXX() convention. Sometimes the routines are available as _builtin___XXXX_chk and sometimes as __builtin_xxx.

I initially thought about limiting the macro magic purely for bionic/arm64, but there does not seem to be a nice way to do this.

Does this mean I need to touch all the .S files that are associated with string.h?
Even for the ones that are NOT for arm64?


Please take a look at:

https://code.google.com/p/android/issues/detail?id=180578

Could you please reiterate why is it not possible to fix this in TSan? If the problem is some call happening before __tsan_init, we just need to call __tsan_init right there. Most (if not all) interceptors have this logic.

Does it still happen if __tsan_init is called from .preinit_array?

Its troublesome before __tsan_init because we lack __thread keyword support in bionic. I had to hack in a workaround in TSAN that does not use intercepted calls. (more on this below)

The majority of the problem actually occurs after __tsan_init has been called.

For example, pthread_create() calls a bunch of routines for the *new thread*, before it has had a chance to initialize its ThreadState. All of these routines running on the new thread are intercepted by tsan, and crashes because ThreadState has not been initialized yet.

A purely tsan-only fix is to "globally ignore" interceptors during troublesome times. This works fine for two threads, but doesn't scale to multiple threads - now you can miss events because they are being ignored.

I also tried putting in "global partial ignores" (i.e. ignore interceptors only on the new thread), but that got really hairy as well.
The fundamental problem is that TSAN is intercepting calls that it has no business intercepting.

For example, the same thing happens during thread join time, when one thread is being deconstructed.

Similar things happen during process exit time where ALL threads are being deconstructed.

Playing around with purely TSAN solution, all I managed to do was move around the actual place of the crash/hang/missed events.

On x86_64/linux, precisely zero calls are intercepted during pthread_create() on the new thread until it has finished initializing its ThreadState.

That does not happen on aarch64/bionic

Oh, the reason why I had to hack in a non-interceptible TLS workaround was that intercepted calls happen during pthread_key maintenance that occurs both during pthread_join time as well as at exit time.

What was happening when I was using pthread_get/setspecific() was that after a thread was destroyed, an intercepted call was GENERATING a NEW key for the dead thread. Hilarity ensued afterwards.

Back to the topic at hand,

does anyone have any ideas on how to limit the bionic headerfile changes purely for aarch64?

https://code.google.com/p/android/issues/detail?id=180578

I *think* it might be better to limit the new internal symbols to 64bit arm parts, but there does not seem to be a nice way to do this (yet).

While my build currently "works", I don't think it will build for other platforms (i.e. 32bit arm, mips etc...)


Thanks

It looks like __aarch64__ is defined for Clang builds. I am not sure about gcc, nor can I say that the bionic folks are going to be pleased with architecture-specific stuff polluting high-level headers.

@11: i think he's confused and expecting __aarch64__ to be defined in code that's built 32-bit.

No, not confused (at least I think)

So the crux of the matter is this.

I need to touch some .S files, which are necessarily architecture specific, to add in the private symbols we need to support TSAN.

I can either touch ALL .S files (arm, arch64, x86 ...), or just the ones that gets used while building a 64 bit device (even if they are 32bit libraries)

Now, is it OKAY for me to touch all the .S files?
If that is the case, then https://code.google.com/p/android/issues/detail?id=180578 is moot.

OTOH, If you guys want me to scope my changes purely for 64bit arm devices, then
https://code.google.com/p/android/issues/detail?id=180578 DOES matter IF there are ever any cases where 64bit library/executable calls out to 32bit library

I hope this is clear

In other words,

if it is okay for me to touch all the variant memset.S and strlen.S files, then I do NOT need to have arch specific exceptions in the new .h files.

The downside is that its gonna be a bigger patch :-/

What do you guys think?

Hi everyone.

I upload a set of patches, all tagged with this bug.
I have no idea if your gerrit handles stacked patches well. (It didn't before)
If it does not, I can always upload a merged patch.

The rough order of patches are

1. new files + pthreads + mman.h
2. system calls (all the .S files and the gensyscalls.py)
3. string.h and friends
4. malloc.h and friends
5. ioctl/tcgetattr

Its not readily apparent what the order of the patches should be.
Here are the change-ids:


    Support for Thread Sanitizer for Bionic
    Change-Id: I724fb8d77059d3e01e1c4336d1aa2c0c550bbb80


    Add new system call weak aliases, modified so that it is not arm64 specific
    Change-Id: I0011bd344ba9ce977e429081a0cb84d8e59463fc


    Touching all of string.h
    Change-Id: I826f328a1849331dc39d5ff6847be02aff2ff1ba

    Now touching malloc/free and friends
    Change-Id: I5a2d07bfcf332556aeb0c3734b5ba122c80ee567

    Now touching ioctl/tcgetattr
    Change-Id: Ia709a5e7acf22239ad57aa22c06a867da6f6ef14

Hi.
I just pushed up a new changeset that combines the 5 mentioned above.


https://android-review.googlesource.com/#/c/162104/

(Apparently gerrit still can't handle stacked changesets without manual intervention.
It marked the 5 prior patches as "unmergeable")

Link to new commit

https://android-review.googlesource.com/#/c/167420/

https://android-review.googlesource.com/#/c/168307/3

has the most recent patch, which is now significantly smaller.

Thank you for your feedback. We assure you that we are doing our best to address the issue reported, however our product team has shifted work priority that doesn't include this issue. For now, we will be closing the issue as won't fix obsolete. If this issue currently still exists, we request that you log a new issue along with latest bug report here https://goo.gl/TbMiIO.

+1
This is crucial.

+1

+100

+1

We have some afterthoughts on this "feature" and as for now we are considering it equally as desired as harmful to the hangouts & bots platform.  
The first thing against that feature is linked to GDPR and overall privacy issues. Each bot with a such power of listening all msgs in the space should have been marked in some way for end users and the bot itself should expose privacy policy, etc. This brings new requirements on operators/owners of the bots.
The second argument against is about assuring that bots have enough power to handle whole volume of space messages and upkeep with good user experience.

This is why we would rather have some kind of anchoring mechanism. Hangouts Chat has already some anchors which triggers builtin functions, like reacting on the Google Map links and overall links. When someone texts a link to a map - a card with a map is presented. In our humble opinion such mechanisms could be extended to bots instead of letting them be flooded by all space messages. Some points of this:
Far less privacy issues - Hangouts Chat could send to the bot subscribed to a specific anchor only the value of the anchor not the whole message of an user.
More focused, hence better maintained bots.
Better manageability - the platform could check whether bots have common anchor.
Better experience for users - end user should have less troubles in understanding why a specific bots have taken an action after his/her message.
As an example, an example bot could have following anchors subscribed:
https://bitbucket.org/user/repo/pull-requests/* - bot could trigger a card with information about specific pull request in Bitbucket
https://jira.atlassian.net/browse/* - bot could trigger a card about specific issue in Jira
SUPPORT-* - same as above but by the issue key.

I wouldn't be opposed to my bot only responding to specific anchors, as long as we have a way of specifying the anchor format ourselves.

Currently, someone has to tag my bot in a room using @jirabot and include a JIRA key in their message.  The bot looks at the message using a regex to find a key such as "AB-1234" corresponding to a JIRA ticket.  We then use an SDK to get the ticket details and post a response to the chat room.  But I don't want to use "https://jira.atlassian.net/browse/...".  Employees in our chat rooms should be able to simply use the format "AB-1234" (two capital letters, dash, up to 4 numbers) so we can look up the ticket info from our self-hosted JIRA implementation.  I would love for everyone to be able to do this without having to tag the bot to get a response.

If not for every message in a room, I would have expected the bot to receive messages in threads they are a part of. If I start a thread and @mention the bot, it only gets that first message and no others in the thread unless you @mention the bot for every message. I expected this because this is how it works for human users with regards to threads. If they've been added to the thread or interacted with it, they start getting notifications for all messages in the threads.

To add to this, the bot doesn't even see messages in threads the bot creates.

This is definitely on our road map. We do plan on unlocking more functionalities for bots. However, we have to figure out the right permission model so users and admins can control such power functionalities. This is a project we are actively working on.

I'd love Hangouts Chat to borrow a page from Gmail:

- spaces -> mailboxes
- messages -> messages
- threads -> threads

Different scopes to limit what information is available to the bot and the use of the users.watch API call to send events to a Pub/Sub topic.

I also would like this. My bot's job is to proactively provide information people in a space need when talking about certain things without them having to ask for it, precisely because they often forget to ask.

+1 ! This feature would open doors for a lot of use cases

+1 please

Please add this feature.  Without this ability, the usefullness of chatbots is severely limited compared to other products like slack

We have a usecase where one space's sole purpose is to have a bot listening in,
if that is easier in terms of right management I could use a specific-space augmentd rights scheme
that would make any message in that space a direct message to the bot regardless of the usage of @bot

+1 Thank you. And not just reading messages but an event trigger when anyone posts into the space/channel.

+1! We could really use an event trigger or webhook each time a message is sent into a Space where the bot is integrated. It's almost against the point of having a bot, at least one that is supposed to act as an assistant within a Chat Space. For our use-case, we would like the bot to be processing messages to understand if it should or can reply. When users need to @ the bot explicitly, that means they either need to do that for every message, or need to know when the bot should respond. But the latter point should be part of the bot's job and not placed on the humans to know that.

Is there another issue for this feature?

Any further updates on this feature?  I guess since it's been open from 2018 wth no progress we can ssume it's at a dead end?